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

Expand dtype accessors #3868

Merged
merged 6 commits into from
Apr 14, 2022

Conversation

oleksandr-pavlyk
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk commented Apr 13, 2022

Description

Added explicit constructor for py::dtype from type number. Also added accessor to retrieve the type_num field of the struct.

While at it, added other accessors.

Added tests.

Suggested changelog entry:

dtype constructor from type number added, accessors corresponding to Python API ``dtype.num``, ``dtype.byteorder``, ``dtype.flags`` and ``dtype.alignment`` added.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

include/pybind11/numpy.h Outdated Show resolved Hide resolved
@oleksandr-pavlyk
Copy link
Contributor Author

Failures are due to error while cloning the Eigen repo.

@Skylion007 Skylion007 merged commit ba7a0fa into pybind:master Apr 14, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 14, 2022
@Skylion007
Copy link
Collaborator

Skylion007 commented Apr 14, 2022

Actually, now that it's merged, not sure the new ctor is really necessary. Wouldn't

dtype::from_args(py::int_(typenum))

have worked? @oleksandr-pavlyk ?

@oleksandr-pavlyk
Copy link
Contributor Author

Actually, since py::from_args is equivalent of calling np.dtype, it dos not work:

In [2]: np.dtype(12)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-dea73ed57aec> in <module>
----> 1 np.dtype(12)

TypeError: Cannot interpret '12' as a data type

In fact modifying the test file:

diff --git a/tests/test_numpy_dtypes.cpp b/tests/test_numpy_dtypes.cpp
index 7de36f2f..8ed6ff8d 100644
--- a/tests/test_numpy_dtypes.cpp
+++ b/tests/test_numpy_dtypes.cpp
@@ -291,7 +291,7 @@ py::list test_dtype_ctors() {
     list.append(py::dtype(names, formats, offsets, 20));
     list.append(py::dtype(py::buffer_info((void *) 0, sizeof(unsigned int), "I", 1)));
     list.append(py::dtype(py::buffer_info((void *) 0, 0, "T{i:a:f:b:}", 1)));
-    list.append(py::dtype(py::detail::npy_api::NPY_DOUBLE_));
+    list.append(py::dtype::from_args(py::int_(static_cast<int>(py::detail::npy_api::NPY_DOUBLE_))));
     return list;
 }

and running raises the same error.

Even if this worked, it would still be less efficient that the added ctor, since the argument Python object needs to be constructed.

@Skylion007
Copy link
Collaborator

I am just wondering if these is a less ambiguous way to build that ctor. Perhaps we could have it take an enum instead of an int or something along those lines? Or maybe it would make more sense to have it be a static factory method. It's a bit weird for the dtype to have an int constructor as it stands currently.

Right now it's a public constructor that could only be used via an implementation detail enum, which is actually really weird. Thoughts @henryiii @rwgk ?

@Skylion007
Copy link
Collaborator

I am just having second thoughts about having this ctor be a part of the public API for dtypes considering the values that are used for it are themselves not public.

@oleksandr-pavlyk oleksandr-pavlyk deleted the expand_dtype_accessors branch April 14, 2022 18:27
@oleksandr-pavlyk
Copy link
Contributor Author

Yes, agreed, the non-public nature of type numbers has bugged me too, but I dismissed it since it is easy to #include "numpy/ndarraytypes.h", or use py::detail.

Also the exposure of num() accessor allows for use of private enums, dtype(dt.num()).

@Skylion007
Copy link
Collaborator

Skylion007 commented Apr 14, 2022

@oleksandr-pavlyk We can revisit this PR in connection to casters we design for #3858 . I'm sure once we see them sketched out we can figure out a better way to either expose the enums or only expose them to the casters.

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 9, 2022
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.

4 participants