-
Notifications
You must be signed in to change notification settings - Fork 129
Mismatched behavior between PyArrayLike1 and PyArrayLike2 when used with floats #520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Icxolu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This mostly looks good to me. I think we can remove the trait here by trying to downcast to an untyped array and checking for success.
Also could you please add a changelog entry for 0.28?
| trait IsNumpyNDArray { | ||
| fn is_numpy_ndarray(&self) -> PyResult<bool>; | ||
| } | ||
|
|
||
| impl<'py> IsNumpyNDArray for Borrowed<'_, 'py, PyAny> { | ||
| fn is_numpy_ndarray(&self) -> PyResult<bool> { | ||
| let py = self.py(); | ||
|
|
||
| static NDARRAY: PyOnceLock<Py<PyAny>> = PyOnceLock::new(); | ||
| let ndarray = NDARRAY | ||
| .get_or_try_init(py, || { | ||
| get_array_module(py)?.getattr("ndarray").map(Into::into) | ||
| })? | ||
| .bind(py); | ||
|
|
||
| self.is_instance(ndarray) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than this, I think we can just try to cast to a Borrowed<'_, '_, PyUntypedArray>.
| // If the input is already an ndarray and `TypeMustMatch` is used then any conversion | ||
| // should be performed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // If the input is already an ndarray and `TypeMustMatch` is used then any conversion | |
| // should be performed. | |
| // If the input is already an ndarray and `TypeMustMatch` is used then no type conversion | |
| // should be performed. |
This pull request attempts to resolve issue #444.
Current behavior
When
PyArrayLike1<f32, TypeMustMatch>is used, arrays constructed usingnp.array([...], dtype='float64')are converted implicitly tof32. This results in a new allocation and a truncation error.Conversely,
PyArrayLike2<f32, TypeMustMatch>behaves differently: arrays constructed usingnp.array([[...], ...], dtype='float64')are not accepted as valid values.I think the current behavior creates confusion and is not consistent with the current documentation, which states:
New behavior
The condition in the following portion of code has been changed to avoid the implicit conversion introduced by
Borrowed::extractcall. The conversion should be performed only when the choice ofDallows type changes or the input object is not a numpy ndarray.rust-numpy/src/array_like.rs
Lines 154 to 163 in 78d5e8d