DEP: Shift correlate mode parsing to C and deprecate inexact matches

[aitikgupta]

Fixes #17476
Also, see #16056

[eric-wieser]

Thanks for the contribution, although this isn't quite right.

I'd actually be very tempted to move this parameter parsing to C, so that it can be handled in exactly the same way as the other arguments like this. Would you be ok to try that instead?

[rossbar]

Agreed - the deprecation should not change the behavior, but warn the user that the behavior will change when the deprecation expires.

A few comments on the tests as well since I had looked at them.

[mattip]

Close/reopen to restart builds

[rossbar]

This change still results in modified behavior. For example, on master np.convolve(*args, mode="silly") will result in mode 1 being used, whereas in the PR, the same call will result in 2. The deprecation should only warn the user of a pending behavior change, not silently change the behavior.

I would recommend getting rid of the default_mode addition and see if you can come up with a minimal change for raising the warning.

[aitikgupta]

This change still results in modified behavior. For example, on master np.convolve(*args, mode="silly") will result in mode 1 being used, whereas in the PR, the same call will result in 2. The deprecation should only warn the user of a pending behavior change, not silently change the behavior.

I would recommend getting rid of the default_mode addition and see if you can come up with a minimal change for raising the warning.

I raised this question here: #17492 (comment)
That is what I intended to start off with, but I'll just stick to the deprecation part here, I'll update the PR. 👍🏼

[eric-wieser]

I think my comment above might have gotten lost:

I'd actually be very tempted to move this parameter parsing to C, so that it can be handled in exactly the same way as the other arguments like this. Would you be ok to try that instead?

[aitikgupta]

I think my comment above might have gotten lost:

I'd actually be very tempted to move this parameter parsing to C, so that it can be handled in exactly the same way as the other arguments like this. Would you be ok to try that instead?

I actually missed it amid suggestions. I wouldn't mind, could you point me to a right path to start digging in?

[eric-wieser]

Sure, here's some pointers: The code that parses the mode argument to correlate is here:

numpy/numpy/core/src/multiarray/multiarraymodule.c

Lines 2897 to 2909 in 565fe77

	static PyObject *
	array_correlate(PyObject *NPY_UNUSED(dummy), PyObject *args, PyObject *kwds)
	{
	PyObject *shape, *a0;
	int mode = 0;
	static char *kwlist[] = {"a", "v", "mode", NULL};
	if (!PyArg_ParseTupleAndKeywords(args, kwds, "OO|i:correlate", kwlist,
	&a0, &shape, &mode)) {
	return NULL;
	}
	return PyArray_Correlate(a0, shape, mode);
	}

My suggestion would be to change that to something like:

if (!PyArg_ParseTupleAndKeywords(args, kwds, "OO|O&:correlate", kwlist, 
                 &a0, &shape, PyArray_CorrelateModeConverter, &mode))

where PyArray_CorrelateModeConverter would look something like:

numpy/numpy/core/src/multiarray/conversion_utils.c

Lines 598 to 673 in 565fe77

	static int clipmode_parser(char const *str, Py_ssize_t length, void *data)
	{
	NPY_CLIPMODE *val = (NPY_CLIPMODE *)data;
	int is_exact = 0;
	if (length < 1) {
	return -1;
	}
	if (str[0] == 'C' || str[0] == 'c') {
	*val = NPY_CLIP;
	is_exact = (length == 4 && strcmp(str, "clip") == 0);
	}
	else if (str[0] == 'W' || str[0] == 'w') {
	*val = NPY_WRAP;
	is_exact = (length == 4 && strcmp(str, "wrap") == 0);
	}
	else if (str[0] == 'R' || str[0] == 'r') {
	*val = NPY_RAISE;
	is_exact = (length == 5 && strcmp(str, "raise") == 0);
	}
	else {
	return -1;
	}
	/* Filters out the case sensitive/non-exact
	* match inputs and other inputs and outputs DeprecationWarning
	*/
	if (!is_exact) {
	/* Numpy 1.20, 2020-05-19 */
	if (DEPRECATE("inexact matches and case insensitive matches "
	"for clip mode are deprecated, please use "
	"one of 'clip', 'raise', or 'wrap' instead.") < 0) {
	return -1;
	}
	}
	return 0;
	}
	/*NUMPY_API
	* Convert an object to NPY_RAISE / NPY_CLIP / NPY_WRAP
	*/
	NPY_NO_EXPORT int
	PyArray_ClipmodeConverter(PyObject *object, NPY_CLIPMODE *val)
	{
	if (object == NULL || object == Py_None) {
	*val = NPY_RAISE;
	}
	else if (PyBytes_Check(object) || PyUnicode_Check(object)) {
	return string_converter_helper(
	object, (void *)val, clipmode_parser, "clipmode",
	"must be one of 'clip', 'raise', or 'wrap'");
	}
	else {
	/* For users passing `np.RAISE`, `np.WRAP`, `np.CLIP` */
	int number = PyArray_PyIntAsInt(object);
	if (error_converting(number)) {
	goto fail;
	}
	if (number <= (int) NPY_RAISE
	&& number >= (int) NPY_CLIP) {
	*val = (NPY_CLIPMODE) number;
	}
	else {
	PyErr_Format(PyExc_ValueError,
	"integer clipmode must be np.RAISE, np.WRAP, or np.CLIP");
	}
	}
	return NPY_SUCCEED;
	fail:
	PyErr_SetString(PyExc_TypeError,
	"clipmode not understood");
	return NPY_FAIL;
	}

[aitikgupta]

Hey @eric-wieser I could use some help here. It took me a while but I was able to shift parameter parsing completely to C, it worked well locally until I did something wrong during the last few changes - build is failing (KeyError of the function name).
I think I worked it out globally using __init__ files, but that wasn't the solid solution.

I'm thinking I should add a Global C Pointer key value in numpy_api?
Pardon if these might be very beginner questions 😅

I've incorporated your suggestion, and haven't changed the behaviour. (You can still review the changes, the build error should be solvable)

[eric-wieser]

We probably don't need to make this part of the public C API - which should also fix your build error

[aitikgupta]

The build works locally, should be fine for review now. 👍🏼

[eric-wieser]

Can you add a test of an invalid integer too?

Also, it may make sense to add a test here too:

numpy/numpy/core/src/multiarray/_multiarray_tests.c.src

Lines 2088 to 2101 in aeb2374

	static PyObject *
	run_clipmode_converter(PyObject* NPY_UNUSED(self), PyObject *args)
	{
	NPY_CLIPMODE mode;
	if (!PyArg_ParseTuple(args, "O&", PyArray_ClipmodeConverter, &mode)) {
	return NULL;
	}
	switch (mode) {
	case NPY_CLIP: return PyUnicode_FromString("NPY_CLIP");
	case NPY_WRAP: return PyUnicode_FromString("NPY_WRAP");
	case NPY_RAISE: return PyUnicode_FromString("NPY_RAISE");
	}
	return PyLong_FromLong(mode);
	}

numpy/numpy/core/src/multiarray/_multiarray_tests.c.src

Lines 2308 to 2310 in aeb2374

	{"run_casting_converter",
	run_casting_converter,
	METH_VARARGS, NULL},

numpy/numpy/core/tests/test_conversion_utils.py

Lines 163 to 174 in aeb2374

	class TestCastingConverter(StringConverterTestCase):
	""" Tests of PyArray_CastingConverter """
	conv = mt.run_casting_converter
	case_insensitive = False
	exact_match = True
	def test_valid(self):
	self._check("no", "NPY_NO_CASTING")
	self._check("equiv", "NPY_EQUIV_CASTING")
	self._check("safe", "NPY_SAFE_CASTING")
	self._check("same_kind", "NPY_SAME_KIND_CASTING")
	self._check("unsafe", "NPY_UNSAFE_CASTING")

[aitikgupta]

I created the tests, but I can't seem to find a way to make PyArray_CorrelatemodeConverter function visible like PyArray_ClipmodeConverter in this file:

numpy/numpy/core/src/multiarray/_multiarray_tests.c.src

Lines 2088 to 2101 in aeb2374

	static PyObject *
	run_clipmode_converter(PyObject* NPY_UNUSED(self), PyObject *args)
	{
	NPY_CLIPMODE mode;
	if (!PyArg_ParseTuple(args, "O&", PyArray_ClipmodeConverter, &mode)) {
	return NULL;
	}
	switch (mode) {
	case NPY_CLIP: return PyUnicode_FromString("NPY_CLIP");
	case NPY_WRAP: return PyUnicode_FromString("NPY_WRAP");
	case NPY_RAISE: return PyUnicode_FromString("NPY_RAISE");
	}
	return PyLong_FromLong(mode);
	}

If I directly #include "conversion_utils.h" it throws a bunch of errors while building, which is understandable.
I think PyArray_ClipmodeConverter is visible to this file as it is a global C API.
Or is there something I'm missing?

[eric-wieser]

I think PyArray_ClipmodeConverter is visible to this file as it is a global C API.

Hmm, that's possible.

If I directly #include "conversion_utils.h" it throws a bunch of errors while building, which is understandable.

Can you give an example of those errors?

[aitikgupta]

Here's the full traceback, it seems like a compiler issue?

In file included from numpy/core/include/numpy/ndarrayobject.h:21:0,
                 from numpy/core/include/numpy/arrayobject.h:4,
                 from numpy/core/src/multiarray/_multiarray_tests.c.src:5:
numpy/core/include/numpy/__multiarray_api.h:1090:12: error: expected identifier or ‘(’ before ‘int’
         (*(int (*)(PyObject *, PyArray_Dims *)) \
            ^
numpy/core/src/multiarray/conversion_utils.h:7:1: note: in expansion of macro ‘PyArray_IntpConverter’
 PyArray_IntpConverter(PyObject *obj, PyArray_Dims *seq);
 ^~~~~~~~~~~~~~~~~~~~~
numpy/core/include/numpy/__multiarray_api.h:1445:10: error: expected ‘)’ before ‘PyArray_API’
          PyArray_API[298])
          ^
numpy/core/src/multiarray/conversion_utils.h:49:1: note: in expansion of macro ‘PyArray_SelectkindConverter’
 PyArray_SelectkindConverter(PyObject *obj, NPY_SELECTKIND *selectkind);
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~

[eric-wieser]

Yeah, looks like your diagnosis is correct; multiarray_tests requires things to be in the public API. I think it's ok to leave things as is for now, thanks for investigating

[aitikgupta]

All builds pass successfully, think we can merge it now?

Moved arg parsing to C

[aitikgupta]

/cc @eric-wieser @rossbar

[eric-wieser]

This ought to have a release note too - sorry for forgetting about this for so long!

[aitikgupta]

sorry for forgetting about this for so long!

That's completely fine :)

[aitikgupta]

@eric-wieser Do you want me to squash the commits?

[mattip]

@eric-wieser could you take one last look?

[mattip]

@aitikgupta this PR was approved but seems to have gotten lost. Now there are merge conflicts. Could you merge with main or rebase it to clear the path for merging it?

@eric-wieser any need for more review?

[eric-wieser]

I don't think this needs any more review; just the merge conflicts resolving after the argument parsing change

[mattip]

rebased to clear merge conflicts

[aitikgupta]

just the merge conflicts resolving after the argument parsing change

@mattip did the heavy lifting before I could jump in :D

[mattip]

Thanks @aitikgupta, sorry it took so long to get this merged.