Skip to content
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

Fix type confusion during downcastsing and add a test case showing how to extract an array from a dictionary. #265

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

adamreichold
Copy link
Member

Fixes #242

@adamreichold
Copy link
Member Author

@davidhewitt @kngwyu I think there is a serious type confusion bug present here. The test added here first failed with

    def test_extract():
        x = np.arange(5)
        d = { "x": x }
>       np.testing.assert_almost_equal(extract(d), 10.0)
E       AssertionError: 
E       Arrays are not almost equal to 7 decimals
E        ACTUAL: 5e-323
E        DESIRED: 10.0

tests/test_ext.py:30: AssertionError
---------------------------------------------------------------------------------------------------------------------------- Captured stderr call ----------------------------------------------------------------------------------------------------------------------------
[examples/simple-extension/src/lib.rs:68] x.dtype() = dtype('int64')

which is surprising as the downcast to PyArray1<f64> should not have worked in the first place due to the dtype.

I think the problem is again that instances of PyArray<T, D> are considered the same type w.r.t. Python for all T, i.e. the macro invocation at

rust-numpy/src/array.rs

Lines 115 to 122 in 2da4787

pyobject_native_type_info!(
PyArray<T, D>,
*npyffi::PY_ARRAY_API.get_type_object(npyffi::NpyTypes::PyArray_Type),
Some("numpy"),
#checkfunction=npyffi::PyArray_Check
; T
; D
);
will imply calling the check function without the generic arguments via https://github.com/PyO3/pyo3/blob/2503a2dd5e9d8b703d3b9c6ce3ce12ce0126affe/src/types/mod.rs#L158 which in our case is npyffi::PyArray_Check which will therefore consider PyArray1<i32> and PyArray1<f64> to be the same type.

I tried to fix this by manually implementing PyTypeInfo and reusing the existing logic of the FromPyObject::extract which includes npyffi::PyArrayCheck but also checks element type and dimensionality.

@adamreichold adamreichold changed the title Add a test case showing how to extract an array from a dictionary. Fix type confusion during downcastsing and add a test case showing how to extract an array from a dictionary. Jan 23, 2022
@adamreichold adamreichold marked this pull request as ready for review January 23, 2022 18:46
@kngwyu
Copy link
Member

kngwyu commented Jan 24, 2022

Oh no!
@davidhewitt If we implement PyTypeInfo manually, should we simplify the macro in pyo3?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, good find!

Now that numpy isn't using the generics for native_type_info I'm definitely tempted to refactor the macros in PyO3. However I don't think we need to rush to do this, because when we try to implement owned PyAny (maybe in 0.17, finally) then we'll need to write a new set of macros anyway. So might as well leave these ones as-is for now.

@adamreichold
Copy link
Member Author

Now that numpy isn't using the generics for native_type_info I'm definitely tempted to refactor the macros in PyO3.

If rust-numpy is the only reason this macro supports generics, then I'd suggest removing as they are a bit of a foot gun. I don''t think it can be correctly implemented without access to the type arguments and their bounds. (Tricks like #checkfunction=Self::checkfunction works to get access to the types, but not to their bounds.)

Downstreams like us who do want to do this should go the manual route so they double-check everything. But then again, the macro itself is not really public documented API in the first place, isn't it?

@adamreichold adamreichold merged commit 7cc945c into main Jan 24, 2022
@adamreichold adamreichold deleted the example-downcast branch January 24, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to extract arrays from a Python object?
3 participants