Use pybind11 in image module by ianthomas23 · Pull Request #26275 · matplotlib/matplotlib · GitHub

Closes the fourth item of issue #23846.

This changes the image module to use pybind11 rather than our array_view classes and direct use of the Python/C API. On my Linux dev machine this brings the _image*.so library 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 pybind11 is to use the py namespace, i.e.

namespace py = pybind11;

but I haven't done this as we already have a py namespace in py_adapters.h and we can't keep both separate as importing some of the agg header files, as needed here, inevitably pulls in py_adapters.h. When we have completed the transition to pybind11 we can change this.

There are a number of utility functions in py_converters.h and .cpp which use PyObject and void pointers. I have started a pybind11 equivalent of this in py_converters_11.h and .cpp, and again when the pybind11 transition 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.

This is more verbose than it used to be, but is the recommended way of checking dtypes in pybind11.

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.

I 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.

This PR looks like a fairly straight forward migration!

Should this be dims[1]?

Suggested change

      
                pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[2], 2};
          
                pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[1], 2};

Yes it should.

Done.

Is this _n_interpolation check still required?

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 to enum mapping.

On reflection, I think the correct approach is to remove it completely from the enum.

Done.

Very nit:

Suggested change

      
                if (ndim < 2 || ndim > 3)
          
                if (ndim != 2 || ndim != 3)

So it's easier to connect the error message with the code.

OK, but it would need to be ndim != 2 && ndim != 3.

Done.

LGTM

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.

It looks like pybind11 generates a function signature, so that could be removed from image_resample__doc__.

ianthomas23 commented Jul 7, 2023

View reviewed changes

ianthomas23 force-pushed the pybind11_image2 branch 2 times, most recently from 22a0788 to ce7afe1 Compare July 8, 2023 14:50

melissawm added the Maintenance label Jul 10, 2023

oscargus added this to the v3.8.0 milestone Jul 13, 2023

thomasjpfan reviewed Jul 15, 2023

View reviewed changes

thomasjpfan approved these changes Jul 27, 2023

View reviewed changes

ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023

oscargus mentioned this pull request Mar 30, 2023

[MNT]: Pybind11 transition plan #23846

Closed

13 tasks done

QuLogic reviewed Sep 14, 2023

View reviewed changes

Comment thread src/_image_wrapper.cpp Outdated

Comment thread src/_image_wrapper.cpp Outdated

Comment thread src/_image_wrapper.cpp Outdated

Comment thread src/_image_wrapper.cpp Outdated

Comment thread src/_image_wrapper.cpp Outdated

Comment thread src/_image_wrapper.cpp

QuLogic reviewed Sep 19, 2023

View reviewed changes

Comment thread src/py_converters_11.cpp Outdated

ianthomas23 added 6 commits September 24, 2023 13:54

Use pybind11 in image module

a3bebed

Be explicit with array dimension types

c14ad96

Review comments

1a3c723

Added curly braces around one-liners

bfaf1a7

Export _InterpolationType rather than interpolation_e

c417369

Miscellaneous review comments

5d90e48

ianthomas23 force-pushed the pybind11_image2 branch from 5469a8e to 5d90e48 Compare September 24, 2023 13:33

QuLogic approved these changes Sep 25, 2023

View reviewed changes

ksunden approved these changes Sep 27, 2023

View reviewed changes

ksunden merged commit 9970395 into matplotlib:main Sep 27, 2023

scottshambaugh mentioned this pull request Jun 24, 2024

[Bug]: Making an RGB image from pickled data throws error #28448

Closed

-                              (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> :

-                              (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> :

-                  if (py_inverse == NULL) {
-                      return NULL;
-                  }
+                  pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[2], 2};

	pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[2], 2};
	pybind11::ssize_t mesh_dims[2] = {dims[0]*dims[1], 2};

+                  if (ndim < 2 || ndim > 3)
+                      throw std::invalid_argument("Input array must be a 2D or 3D array");
-                  if (params.interpolation < 0 || params.interpolation >= _n_interpolation) {

Use pybind11 in image module by ianthomas23 · Pull Request #26275 · matplotlib/matplotlib · GitHub

Repository navigation

Use pybind11 in image module - #26275

Use pybind11 in image module#26275ksunden merged 6 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom ianthomas23:pybind11_image2ianthomas23/matplotlib:pybind11_image2Copy head branch name to clipboard

Conversation

ianthomas23 commented Jul 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscargus commented Jul 13, 2023

Uh oh!

ianthomas23 commented Jul 13, 2023

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QuLogic commented Sep 14, 2023

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Use pybind11 in image module#26275
ksunden merged 6 commits into
matplotlib:mainfrom
ianthomas23:pybind11_image2