MAINT,API: Introduce __numpy_dtype__ and fix dtype attribute recursion#30179
MAINT,API: Introduce __numpy_dtype__ and fix dtype attribute recursion#30179charris merged 3 commits intonumpy:mainfrom
Conversation
This is a lot more complex then I expected. We, correctly, deprecated
`.dtype` attribute lookup recursion, however...
The code still had a `try/except` that still recursed and that try
except carried meaning at least in a weird path of:
```
class mydtype(np.void):
dtype = ...
```
Now does that make sense? Maybe not... we also fall back to object
in some paths which should have been broken when a dtype attribute
existed on a type but it is a property/descriptor.
So, this removes the recursion, but adds a check for `__get__` to
filter out those cases, this is something we successfully did for
other protocols `__array__`, `__array_interface__`, etc.
Signed-off-by: Sebastian Berg <[email protected]>
|
Would adding a |
|
Yeah, that is a nice thought, we should maybe add it to deprecate the use of the dtype on the instance. I guess that makesme lean to: worth an issue/followup, but maybe not needed here? |
| For array-like objects we encourage you to implement ``__numpy_dtype__`` | ||
| with a warning or error to _prevent_ using e.g. ``dtype=dataframe`` in | ||
| NumPy functions (it may be good to go via a Deprecation). |
There was a problem hiding this comment.
Do we really need other array libraries to take action? If they don't do anything, the status-quo for these types of array-likes is maintained (works or doesn't work depending on the case).
A warning / deprecation / error can be handled centrally in numpy. You could even make this behavior dependent on whether the passed object is array-like and/or whether they return a numpy dtype.
There was a problem hiding this comment.
Let me rephrase it to say that you could do this. NumPy would indeed deprecate it for you as soon as we actually deprecate .dtype.
I agree with that deprecation, but think we may want to wait a little bit, implementing it in the downstream library would be an early opt-in effectively.
inakleinbottle
left a comment
There was a problem hiding this comment.
I had a look through the changes to the C code. I can't see any obvious issues in the C code.
| dt_instance = dt() | ||
| dt_instance.dtype = dt | ||
| with pytest.raises(RecursionError): | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
Does this behavior change deserve a release note?
There was a problem hiding this comment.
🤷 I don't think recursion errors are a type of error that anyonen should ever run into really.
ngoldbaum
left a comment
There was a problem hiding this comment.
LGTM, just one small comment about whether the changing error type deserves a release note
|
Thanks Sebastian. |
|
I guess I'll do the typing stuff for this tomorrow then (in 12 hours or so). I assume that branching 2.4 is still planned for his weekend? |
|
Maybe we could add a And how about also adding it to the scalar-type classes, e.g.i.e. That way we'd be able to simplify # numpy._typing
class _SupportsDType[DTypeT: np.dtype = np.dtype](Protocol):
@property
def dtype(self) -> DTypeT: ...
type _VoidDTypeLike = # <omitted spaghetti>
type _DTypeLike[ScalarT: np.generic] = type[ScalarT] | np.dtype[ScalarT] | _SupportsDType[np.dtype[ScalarT]]
# numpy.typing
type DTypeLike = np.dtype | type | str | _SupportsDType | _VoidDTypeLiketo just # numpy._typing
class _SupportsDType[DTypeT: np.dtype = np.dtype](Protocol):
@property
def __numpy_dtype__(self) -> DTypeT: ...
type _VoidDTypeLike = # <omitted spaghetti>
type _DTypeLike[ScalarT: np.generic] = _SupportsDType[np.dtype[ScalarT]]
# numpy.typing
type DTypeLike = type | str | _DTypeLike | _VoidDTypeLike(this uses Python 3.13+ PEP 696 syntax for aesthetics) Typing aside, I suppose it'd also be nice if we'd "practice what we preach", analogous to e.g. |
Possible, but is it worth it? You need to make a funky dance with a custom descriptor stuffed into the I do also do like to (in principle!) allow tying scalar types to dtypes that do not allow adding a custom attribute like this (becuase they are defined by a 3rd party). I.e. that still needs something like the EDIT: Although, I guess typing wise the last thing might not matter too much, in the sense that I am not sure user dtypes are supported typing wise to begin with... |
Only if we are ready. |
Yea, that's right. It'd be similar to that The approach that we currently use to statically determine a scalar type's associated dtype, via its |
Can't we use |
|
You are confusing scalar types and dtypes. |
Ah yea, you're right; classic mistake :P |
numpy#30179) * MAINT,API: Introduce __numpy_dtype__ and fix dtype attribute recursion This is a lot more complex then I expected. We, correctly, deprecated `.dtype` attribute lookup recursion, however... The code still had a `try/except` that still recursed and that try except carried meaning at least in a weird path of: ``` class mydtype(np.void): dtype = ... ``` Now does that make sense? Maybe not... we also fall back to object in some paths which should have been broken when a dtype attribute existed on a type but it is a property/descriptor. So, this removes the recursion, but adds a check for `__get__` to filter out those cases, this is something we successfully did for other protocols `__array__`, `__array_interface__`, etc. Signed-off-by: Sebastian Berg <[email protected]> * DOC: Add release note snippet * DOC: Make release note more precise, array-likes don't need the new attr --------- Signed-off-by: Sebastian Berg <[email protected]>
numpy#30179) * MAINT,API: Introduce __numpy_dtype__ and fix dtype attribute recursion This is a lot more complex then I expected. We, correctly, deprecated `.dtype` attribute lookup recursion, however... The code still had a `try/except` that still recursed and that try except carried meaning at least in a weird path of: ``` class mydtype(np.void): dtype = ... ``` Now does that make sense? Maybe not... we also fall back to object in some paths which should have been broken when a dtype attribute existed on a type but it is a property/descriptor. So, this removes the recursion, but adds a check for `__get__` to filter out those cases, this is something we successfully did for other protocols `__array__`, `__array_interface__`, etc. Signed-off-by: Sebastian Berg <[email protected]> * DOC: Add release note snippet * DOC: Make release note more precise, array-likes don't need the new attr --------- Signed-off-by: Sebastian Berg <[email protected]>
This is a lot more complex then I expected. We, correctly, deprecated
.dtypeattribute lookup recursion, however...The code still had a
try/exceptthat still recursed and that try except carried meaning at least in a weird path of:Now does that make sense? Maybe not... we also fall back to object in some paths which should have been broken when a dtype attribute existed on a type but it is a property/descriptor.
So, this removes the recursion, but adds a check for
__get__to filter out those cases, this is something we successfully did for other protocols__array__,__array_interface__, etc.