API,BUG,DEP: treat trailing comma as a tuple and thus a structured dtype.#25434
Conversation
mhvk
force-pushed
the
dtype-from-string-with-trailing-comma
branch
from
6bbb120 to
1c0c76c
Compare
seberg
added
the
56 - Needs Release Note.
mhvk
changed the title
mhvk
force-pushed
the
dtype-from-string-with-trailing-comma
branch
2 times, most recently
from
1b11192 to
41031e0
Compare
|
Added a release note... |
mhvk
force-pushed
the
dtype-from-string-with-trailing-comma
branch
from
41031e0 to
3d7540e
Compare
seberg
removed
the
56 - Needs Release Note.
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks LGTM, just nits, this is OK to put in to me.
If someone has an idea to shorten the release note, I wouldn't mind.
Otherwise, I am ambivalent towards the deprecation, but at least if it is tedious to make (2)i (without any comma, I think it takes a different code path!) work, neither do I care.
I would appreciate cleaning up the C-calling code a bit (cache _commastring not the module).
(To me the Python side parsing code looks a bit overly complicated, but not a problem here...)
| } | ||
| if (PyList_GET_SIZE(listobj) == 1) { | ||
| res = _convert_from_any(PyList_GET_ITEM(listobj, 0), align); | ||
| else if (PyList_Check(parsed) && PyList_GET_SIZE(parsed) >= 1) { |
There was a problem hiding this comment.
| else if (PyList_Check(parsed) && PyList_GET_SIZE(parsed) >= 1) { | |
| else if (PyList_Check(parsed)) { |
I suppose length 0 is impossible here anyway? I suspect we could remove the size check.
There was a problem hiding this comment.
I think the length checks were mainly meant to guard against silly mistakes in the python code. My sense would be to leave it in.
…ype.
Previously, `np.dtype("i")` and `np.dtype("i,")` were treated as
identical, even though `np.dtype("i,i")` creates a structured dtype
with two fields. Now, `np.dtype("i,")` will create a structured dtype
as well, with a single field.
At the same time, the use of `np.dtype("(2)i,")` to create a subarray
with 2 elements is deprecated; one should use `np.dtype("(2,)i")` or
`np.dtype("2i")` instead. This is consistent with `np.dtype("(2)i")`
raising an exception.
mhvk
force-pushed
the
dtype-from-string-with-trailing-comma
branch
from
3d7540e to
86b39ab
Compare
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks, LGTM now. Will wait a bit in case someone has a comment or just feels like pressing the green button :).
|
Thanks @mhvk! |
* Fixes scipy#19737 * the deprecation is obviously coming from numpy/numpy#25434; I picked one of the alternatives Marten suggested there and it passes locally on x86_64 Linux with NumPy `main` and `1.26.2` [skip cirrus]
* Fixes #19737 * the deprecation is obviously coming from numpy/numpy#25434; I picked one of the alternatives Marten suggested there and it passes locally on x86_64 Linux with NumPy `main` and `1.26.2`
3 participants
Previously,
np.dtype("i")andnp.dtype("i,")were treated as identical, even thoughnp.dtype("i,i")creates a structured dtype with two fields. Now,np.dtype("i,")will create a structured dtype as well, with a single field.At the same time, the use of
np.dtype("(2)i,")to create a subarray with 2 elements is deprecated; one should usenp.dtype("(2,)i")ornp.dtype("2i")instead. This is consistent withnp.dtype("(2)i")raising an exception; in that sense, the previously allowed usage was a bug.fixes #25429