BUG: allow registration of hard-coded structured dtypes#17320
BUG: allow registration of hard-coded structured dtypes#17320mattip merged 4 commits intonumpy:masterfrom
Conversation
This also includes `NPY_ITEM_IS_POINTER`. A previous changed raised an error when this happened, but some downstream libraries use it to create a custom dtype with a single object field. It seems acceptable to create such a dtype if (and only if) that dtype hardcodes names and fields at creation time, so this change allows that, but otherwise keeps the error intact. This should work fine, although some care may be required
This allows running `xpress` on current master. `xpress` copies the type from `np.dtype(object)` rather than using `&PyArrayDescr_Type`. That seems overall fine, we can just replace it. The only reason for this check is to ensure that the user does not override our updated `type(user_dtype)` and thus corrupting it.
|
@octachrome this PR should make I am slightly worried about missing reference counting corner cases with a dtype such as |
|
Does this need a backport? |
|
No need to backport, this is just fighting with more exotic user-dtypes in master. Moving where |
|
Thanks again for taking the time to look at our module. I will pass this on to the lead dev. |
Do we need to address this in |
We upload weekly builds to https://anaconda.org/scipy-wheels-nightly/numpy, so you can install them with |
|
Right now there is hopefully nothing you need to do in xpress. This PR needs to go in, before you can test anything. Testing against the |
|
Importing |
|
@octachrome no need to try to debug it right now, it won't work unless you install from source with this PR in. What you could do maybe is that errors happening during dtype registration seem to be ignored. The cuase of the error you see is actually that registering the new dtype failed earlier. |
|
You're right, we aren't handling the case where |
|
Codecov seems to think the new code is not covered in a test. If that is correct, it would be nice to have a test that hits this path. |
|
Yes, tests added. I didn't have the idea to just copy a full blueprint to make the test, but as is, its probably OK and not super long. Interesting side issue, because |
This includes a few error paths. Note that the test creates a new dtype ever time it is run, and dtypes in NumPy are persistent so that this leaks references.
6601a10 to
e5f2ce3
Compare
Is this about placing the right INC/DECREF in the dtype functions? At the moment we do have a correct balance of these for some of our tests. But obviously I'm interested in any insight you have about this. |
|
Thanks @seberg |
It is probably fine if user dtypes have a hardcoded structure (although I am not sure we can allow them to support byte-swapping). In that case, it is even possible for them to include an
objectfield.One reason for this error was to find out if someone uses this type of thing, and
xpressdoes create "typed" object dtypes in this manner, so relax it to allowxpressto keep working.xpressruns into the error message I tried to make a bit clearer (not filling inPyArrayDescr_Typebefore registration). That is unfortunate, but I am not sure I want to check for it at every singlenp.dtype()call (or similar), so that I think we may just need to ask them to move the code and release a new version.I tested this using
xpress, not sure how easy it is to add another user dtype to test these error paths.