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

ianthomas23 · 2023-07-07T09:41:18Z

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.

ianthomas23 · 2023-07-07T09:42:24Z

src/_image_wrapper.cpp

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


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.

oscargus · 2023-07-13T07:21:23Z

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

ianthomas23 · 2023-07-13T11:42:49Z

The tests are related, but I am away from dev machine at SciPy this week. I'll return to this next week.

thomasjpfan

This PR looks like a fairly straight forward migration!

thomasjpfan · 2023-07-15T15:49:11Z

src/_image_wrapper.cpp

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


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.

thomasjpfan · 2023-07-15T15:54:42Z

src/_image_wrapper.cpp

+    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) {


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.

thomasjpfan · 2023-07-15T15:57:27Z

src/_image_wrapper.cpp

-            &params.alpha, &convert_bool, &params.norm, &params.radius)) {
-        return NULL;
-    }
+    if (ndim < 2 || ndim > 3)


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.

src/_image_wrapper.cpp

thomasjpfan

LGTM

thomasjpfan · 2023-07-25T15:58:30Z

src/_image_wrapper.cpp

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


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.

src/_image_wrapper.cpp

QuLogic · 2023-09-14T08:15:47Z

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

src/py_converters_11.cpp

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

QuLogic reviewed Sep 14, 2023

View reviewed changes

QuLogic reviewed Sep 19, 2023

View reviewed changes

src/py_converters_11.cpp Outdated Show resolved Hide resolved

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

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

Uh oh!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants