Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use pybind11 in image module#26275
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| (type == NPY_INT16) ? resample<agg::gray16> : | ||
| (type == NPY_FLOAT32) ? resample<agg::gray32> : | ||
| (type == NPY_FLOAT64) ? resample<agg::gray64> : | ||
| (dtype.is(pybind11::dtype::of<std::uint8_t>())) ? resample<agg::gray8> : |
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.
This is more verbose than it used to be, but is the recommended way of checkingdtypes inpybind11.
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.
At SciPy Conf 2023, I asked Henry (pybind11 maintainer) if there was a less verbose way to do this type of check and he said this was the best way.
22a0788 toce7afe1CompareI cannot really review it properly, but I am positive to the effort! Are the failing tests somehow related? It seems like they may be, but then they only show up for some Python versions... |
The tests are related, but I am away from dev machine at SciPy this week. I'll return to this next week. |
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.
This PR looks like a fairly straight forward migration!
src/_image_wrapper.cpp Outdated
| if (py_inverse == NULL) { | ||
| return NULL; | ||
| } | ||
| pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[2], 2}; |
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.
Should this bedims[1]?
| pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[2],2}; | |
| pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[1],2}; |
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.
Yes it should.
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.
Done.
| if (params.interpolation < 0 || params.interpolation >= _n_interpolation) { |
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.
Is this_n_interpolation check still required?
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.
That is a good question. The value is only ever used internally (between the Python and C++ code) and there is no way for a user to get this value as it is not part of the_interpd_str toenum mapping.
On reflection, I think the correct approach is to remove it completely from the enum.
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.
Done.
src/_image_wrapper.cpp Outdated
| ¶ms.alpha, &convert_bool, ¶ms.norm, ¶ms.radius)) { | ||
| return NULL; | ||
| } | ||
| if (ndim < 2 || ndim > 3) |
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.
Very nit:
| if (ndim<2 || ndim>3) | |
| if (ndim!=2 || ndim!=3) |
So it's easier to connect the error message with the code.
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.
OK, but it would need to bendim != 2 && ndim != 3.
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.
Done.
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM
| (type == NPY_INT16) ? resample<agg::gray16> : | ||
| (type == NPY_FLOAT32) ? resample<agg::gray32> : | ||
| (type == NPY_FLOAT64) ? resample<agg::gray64> : | ||
| (dtype.is(pybind11::dtype::of<std::uint8_t>())) ? resample<agg::gray8> : |
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.
At SciPy Conf 2023, I asked Henry (pybind11 maintainer) if there was a less verbose way to do this type of check and he said this was the best way.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
It looks like pybind11 generates a function signature, so that could be removed from |
Uh oh!
There was an error while loading.Please reload this page.
5469a8e to5d90e48Compare
Closes the fourth item of issue#23846.
This changes the
imagemodule to usepybind11rather than ourarray_viewclasses and direct use of the Python/C API. On my Linux dev machine this brings the_image*.solibrary down from 1.8 MB to 1.0 MB.I've taken the usual approach of the minimal set of changes to switch to
pybind11. This means that in some places we are still passing around pointers to array buffers an integer array dimensions. We could consider improving that in future, but it might be better to leave it as it is using the "if it ain't broke" principle.The standard approach to using
pybind11is to use thepynamespace, i.e.but I haven't done this as we already have a
pynamespace inpy_adapters.hand we can't keep both separate as importing some of the agg header files, as needed here, inevitably pulls inpy_adapters.h. When we have completed the transition topybind11we can change this.There are a number of utility functions in
py_converters.hand.cppwhich usePyObjectandvoidpointers. I have started apybind11equivalent of this inpy_converters_11.hand.cpp, and again when thepybind11transition is completed we can delete the old ones.With regard to code formatting, I have just done what I would do instinctively without thinking about it much and I am very happy to make changes if anyone has a strong opinion.