BUG: Fix misuse of .names and .fields in various places #14290
Conversation
This comment has been minimized.
This comment has been minimized.
|
pytest 5.1.0 crashes on python3.5-dbg (not limited to numpy --- occurs on all projects I know), triggers some fatal assert in Python ast parser |
This comment has been minimized.
This comment has been minimized.
|
close/reopen |
This replaces some more uses of `bool(dt.fields)` and `bool(dt.names)` with `dt.names is not None`. `dt.fields is not None` would have worked too, but checking `.names` is more prevalent elsewhere
Previously attempting to access a field of such an array (such as when printing it!) would result in `ValueError: Changing the dtype of a 0d array is only supported if the itemsize is unchanged`.
Previously this would fail with `ValueError: could not assign tuple of length 2 to structure with 3 fields.`, now it raises `NotImplementedError`.
6a568f6 to
d110d66
Compare
Replacing empty tuples with `None` is a bad idea, and just results in an API that is hard to consume - especially since the behavior was never documented. This affects `get_names`, `get_names_flat`, and `get_fieldstructure`.
In these instances the behavior isn't changed, since the for loop below acts like an if. However, in general this is an antipattern that crashes on 0-field structured types, and is warned against in the docs. If we remove instances of the antipattern, it will hopefully not reappear via copy-paste code.
Without this change, `np.dtype('V0')` and `np.dtype([])` produced types with the same name, which was misleading as they are different types.
This is mostly cosmetic.
Also adjust the code to more clearly indicate what actually happens. The behavior is identical before and after this patch.
…rrayprint No behavior change here
Previously passing `dtype=[], names=['a']` would append an extra field, even though `dtype=['a'], names=['b', 'c']` does not.
…mtxt This only affects arrays with `dtype([])`, but also follows the recommended way to check for structured arrays in our docs
This check would fail on the structured type `np.dtype([])`. No test, since I don't really understand mrecords
It's not clear that these have any visible effect, but we should be consistent with how we detect structured types.
fdf4bad to
0f5e376
Compare
|
LGTM. There are probably still things broken with zero-field dtypes, eg perhaps the view issue from #14310, but its good to fix all these. |
|
@ahaldane Do you think this should go in before your fixes? |
|
I don't think it matters, this doesn't overlap in its current form. |
|
@eric-wieser If you are happy with this I'll go ahead and merge. Looks like it might close some issues, are they included in the commit messages? |
|
I haven't found any issues this will close yet - they're deep enough corner cases they were probably never reported. If you put this in (without squashing), I'll kick off backport PRs for 1.16 and 1.17 tonight (assuming you want both) |
|
@seberg has marked this for his review, so I'll wait on that. So tomorrow night. I figure if the bugs are corner cases, any bugs in the fixes are likely be corner cases too :) |
|
@eric-wieser Merged. |
Easiest to review commit-by-commit.
Done with the 1.16.x branch point as its base, so it can be easily backported to either 1.16 or 1.17 if desired