Replace use of Python/C API with numpy::array_view in _tri.cpp and qhull_wrap.c

[ianthomas23]

_tri.cpp and qhull_wrap.c use the Python/C API to create numpy arrays, e.g.:

matplotlib/src/tri/_tri.cpp

Lines 638 to 639 in bc97294

	PyArrayObject* segs = (PyArrayObject*)PyArray_SimpleNew(
	2, segs_dims, NPY_DOUBLE);

It would be better to use our numpy::array_view wrappers instead, e.g.

matplotlib/src/_contour.cpp

Line 440 in bc97294

numpy::array_view<double, 2> vertices(vertices_dims);

as it is simpler and deals with reference counting and raising exceptions for us.

Self-assigning.

[rajpratyush]

@ianthomas if you are not working on the issue can I take it up ?

[ianthomas23]

@rajpratyush I haven't started to work on this issue so if you would like to then please go ahead. I will unassign myself.

My issue report gave an example of what to do for numpy arrays that are created in C/C++ and returned to Python. I've realised that qhull_wrap.c also needs updating for numpy arrays that are passed in the other direction. For example

matplotlib/src/qhull_wrap.c

Lines 275 to 283 in 200ebe1

	if (!PyArg_ParseTuple(args, "OO", &xarg, &yarg)) {
	PyErr_SetString(PyExc_ValueError, "expecting x and y arrays");
	return NULL;
	}
	xarray = (PyArrayObject*)PyArray_ContiguousFromObject(xarg, NPY_DOUBLE,
	1, 1);
	yarray = (PyArrayObject*)PyArray_ContiguousFromObject(yarg, NPY_DOUBLE,
	1, 1);

needs to be more like

matplotlib/src/_contour_wrapper.cpp

Lines 36 to 44 in 200ebe1

	if (!PyArg_ParseTuple(args, "O&O&O&O&O&l",
	&x.converter_contiguous, &x,
	&y.converter_contiguous, &y,
	&z.converter_contiguous, &z,
	&mask.converter_contiguous, &mask,
	&convert_bool, &corner_mask,
	&chunk_size)) {
	return -1;
	}

If you have any questions then ask away. I know this part of the codebase pretty well.

[rajpratyush]

@ianthomas23 surely I would like to give it a try.

[ianthomas]

Prettty sure that’s @ianthomas23 you’re looking for

…

On Sun, Mar 14, 2021 at 11:54 PM Pratyush Raj ***@***.***> wrote: @ianthomas <https://github.com/ianthomas> if you are not working on the issue can I take it up ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19627 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACJANNDTBUZSSODN2VIADTDWAGNANCNFSM4YRHL23Q> .

[QuLogic]

Converting qhull_wrap.c ostensibly means converting it to C++. This is not too difficult, though it does entail fixing some problems with differing standards. But do we also want to convert to Qhull's C++ API? Then we can drop the gotos, probably.

[ianthomas23]

I'd intended the minimal changes of renaming qhull_wrap.c to qhull_wrap.cpp and just changing the array reading/writing. So the wrapper would be mostly C but compiled with a C++ compiler. Might also need a few tweaks like extern "C" wrapping of include files, etc. This could be done without having to understand qhull's slightly quirky API.

I hadn't considered switching to qhull's C++ API. It seems to me to be a work in progress and not fully nailed down, so unnecessarily risky for mpl to adopt.

[QuLogic]

I opened a separate PR as it was quite a bit long to add to the first one. I didn't bother with the Qhull C++ API, which seems difficult to decipher.

[ianthomas23]

OK. I'm probably the most appropriate person to review both PRs so I'll assign myself.