Repository navigation
MAINT: Express PyArray_DescrAlignConverter in terms of _convert_from_any - #15287
Conversation
b06f0ad to
ee0cb5d
Compare
This also fixes an inconsequential bug, meaning the following (scary) code no longer asserts
```
np.typeDict['mypoint`] = [('a', 'i1'), ('b', 'i2')]
assert np.dtype('mypoint', align=True).isalignedstruct
```
I'm not going to legitimize this sort of thing with a test though.
ee0cb5d to
b36db66
Compare
| /* | ||
| * Probably only ever dispatches to `_convert_from_type`, but who | ||
| * knows what users are injecting into `np.typeDict`. | ||
| */ | ||
| return _convert_from_any(item, align); |
There was a problem hiding this comment.
If we want to keep the old behavior, we could change this to
| /* | |
| * Probably only ever dispatches to `_convert_from_type`, but who | |
| * knows what users are injecting into `np.typeDict`. | |
| */ | |
| return _convert_from_any(item, align); | |
| return _convert_from_any(item, 0); |
|
Can we deprecate that typeDict nonsense? (I know, not here) And with deprecate it, I mean just freeze the content (in case we need it internally right now) unceremoniously and give an error for people attempting to do such evil things? |
|
Perhaps replace it with a I think @moble might be inserting a quaternion type string in there, but maybe not. Either way, I'm not sure string definitions of new dtypes are something we want to encourage. |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Looks good to me, much less error prone, will merge soon probably. I guess by now convert_from_any and _arraydescr_run_converter are practically identically, and convert_from_any should be removed, but I am sure you are planning that in a followup.
|
Hmm, you are right. I had checked rational, quaternion does use it currently. I do not like the string usage much, and would prefer if we do not use it for new stuff. One thing we could do is to add "old style" dtypes ourselves, removing the need to modify it. (OTOH, it will still break quaternions/give a warning at least). |
|
At a minimum we could require that the things in that dict are dtype objects or |
Except crucially, the former can't be trusted to set an exception, while the latter can. |
|
I'm afraid I'm completely lost as to what's going on here, or how it relates to quaternion, but I'm entirely willing to change any code over there to work with newer approaches you guys are using. Please just let me know what I'll need to do. |
|
Oh, I was basically saying it would be nice if we remove the hook that currently allows your users to write |
|
In any case, let me merge this. Thanks Eric. Before I realized how weird exactly that fixed path was, I thought about adding a test, but nvm. that... |
|
xref gh-15296, which is an issue for the above string registration method. |
3 participants
Builds on gh-15284, will need rebasing once that gets merged.
This also fixes an inconsequential bug, meaning the following (scary) code no longer asserts
I'm not going to legitimize this sort of thing with a test though.