Repository navigation
DEP: Deprecate collapsing shape-1 dtype fields to scalars. - #13326
Conversation
There was a problem hiding this comment.
This should be a future warning, I believe we have a macro for that too.
Congrats, you found a bug in For the time being, you should probably just replace |
There was a problem hiding this comment.
To work around #13327 you need to special-case c == 1 here
There was a problem hiding this comment.
Nevermind - the actual problem is the test is bad - it should be checking ret.base is a or ret is a - or in a shorter form, ret.view().base is a
There was a problem hiding this comment.
Switching to either form still fails for me locally.
|
Looking more closely - this is a bug / missing optimization in import numpy.lib.recfunctions as rfn
dt = np.dtype([('f', (int, (2, 2)))])
arr = np.zeros(10, dt)
arr2 = rfn.structured_to_unstructured(arr)
assert arr2.base is arr # fails |
|
Rebasing on #13332 should fix things |
|
I'll wait for it to be merged first? Please ping me when it is :) |
charris
changed the title
|
#1332 was merged, this needs a rebase |
|
thanks, rebased. |
|
Looks good to me. Do we need to pass this by the mailing list? |
Probably worth doing that. The proposed changes make sense to me, but it never hurts to see if anyone else has thoughts on the matter. |
eric-wieser
left a comment
eric-wieser
left a comment
There was a problem hiding this comment.
Needs a test in test_deprecations.py too, I think
b491878 to
783f8d1
Compare
|
added test in test_deprecations emailed the mailing list |
There was a problem hiding this comment.
Based on your test changes, it might be worth a remark on the message that this also applies to / is caused by format strings like "1f4". I don't think you need to go to the effort of working out whether the user used tuples or strings though.
There was a problem hiding this comment.
good point, edited.
|
In response to the mailing list: very much in favour! |
|
Looks good to me. Can this make 1.17? |
There was a problem hiding this comment.
I think this needs to be (len(_mask.dtype),) else it will run into the deprecation warning here when len(_mask.dtype) == 1
Currently, a field specified as `[(name, dtype, 1)]` is interpreted as a scalar field (i.e., the same as `[(name, dtype)]` or `[(name, dtype, ()]`). This now raises a FutureWarning; in a future version, it will be interpreted as a shape-(1,) field, i.e. the same as `[(name, dtype, (1,))]` (consistently with `[(name, dtype, n)]` with `n>1`, which is already equivalent to `[(name, dtype, (n,)]`).
| pass | ||
| hasmasked = _mask.view((bool, (len(_mask.dtype) or 1))).any() | ||
| tp_len = len(_mask.dtype) | ||
| hasmasked = _mask.view((bool, ((tp_len,) if tp_len else ()))).any() |
There was a problem hiding this comment.
This if condition should be _mask.dtype.fields is None, else it blows up on structured types with 0 fields. Arguably out of scope, but may as well fix this.
If you're looking for another deprecation for another PR, len(dtype) is looking like a bad idea on non structured dtypes... If not, I'll probably do that myself.
There was a problem hiding this comment.
Although actually the function is crap anyway and should be using one of the recursive helpers in ma/core that ors together all the fields
There was a problem hiding this comment.
So should I update this PR, or just leave it as it is and let you do a followup? (I would prefer the latter :))
There was a problem hiding this comment.
I suppose you could argue that this PR is about fixing things for length-1 subarrays, and you've succeeded even without fixing this. I think fine to put in as is, assuming we can sort out the ci failure by restarting
|
CI is passing. Any more comments? |
|
Not on my side. |
|
Still needs a test in |
|
Let's put this in - more than one core dev in favor of the deprecation, and at worst this will create too much noise. Thanks @anntzer. I look forward to never being bitten by this again! |
See discussion at #13112.
Currently, a field specified as
[(name, dtype, 1)]is interpreted asa scalar field (i.e., the same as
[(name, dtype)]or[(name, dtype, ()]). This now raises a DeprecationWarning; in a future version, itwill be interpreted as a shape-(1,) field, i.e. the same as
[(name, dtype, (1,))](consistently with[(name, dtype, n)]withn>1, whichis already equivalent to
[(name, dtype, (n,)]).@eric-wieser: This currently fails test_recfunctions::TestRecFunctions::test_structured_to_unstructured (apparently an extra copy is now being made), but it's not clear to me why.