DEP: Shift correlate mode parsing to C and deprecate inexact matches by aitikgupta · Pull Request #17492 · numpy/numpy

aitikgupta · 2020-10-07T14:47:51Z

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?

numpy/core/numeric.py

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.

numpy/core/tests/test_numeric.py

mattip · 2020-10-08T05:09:46Z

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 · 2020-10-08T16:43:52Z

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 · 2020-10-08T16:57:21Z

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 · 2020-10-08T17:01:10Z

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 · 2020-10-08T17:02:17Z

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 · 2020-10-09T13:50:58Z

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

numpy/core/src/multiarray/conversion_utils.c

aitikgupta · 2020-10-09T14:30:11Z

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

numpy/core/src/multiarray/conversion_utils.c

eric-wieser · 2020-10-10T10:11:05Z

numpy/core/tests/test_numeric.py

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")

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?

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?

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); ^~~~~~~~~~~~~~~~~~~~~~~~~~~

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

numpy/core/src/multiarray/conversion_utils.c

aitikgupta · 2020-10-12T17:02:05Z

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

Moved arg parsing to C

aitikgupta · 2020-11-04T03:36:52Z

/cc @eric-wieser @rossbar

numpy/core/src/multiarray/conversion_utils.c

eric-wieser

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

aitikgupta · 2021-01-19T11:56:48Z

sorry for forgetting about this for so long!

That's completely fine :)

doc/release/upcoming_changes/17492.deprecation.rst

numpy/core/src/multiarray/conversion_utils.c

numpy/core/tests/test_numeric.py

aitikgupta · 2021-01-19T13:25:58Z

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

mattip · 2021-02-06T21:12:04Z

@eric-wieser could you take one last look?

mattip · 2021-03-18T16:58:25Z

@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 · 2021-03-18T17:02:29Z

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

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

mattip · 2021-03-18T19:04:31Z

rebased to clear merge conflicts

aitikgupta · 2021-03-18T20:17:22Z

just the merge conflicts resolving after the argument parsing change

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

numpy/core/src/multiarray/conversion_utils.c

mattip · 2021-03-18T20:49:17Z

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

github-actions bot added the 07 - Deprecation label Oct 7, 2020

eric-wieser reviewed Oct 7, 2020

View reviewed changes

numpy/core/numeric.py Outdated Show resolved Hide resolved

numpy/core/numeric.py Outdated Show resolved Hide resolved

rossbar reviewed Oct 7, 2020

View reviewed changes

numpy/core/tests/test_numeric.py Outdated Show resolved Hide resolved

numpy/core/tests/test_numeric.py Outdated Show resolved Hide resolved

aitikgupta force-pushed the unwanted-mode-dep branch from f177eef to 7708fe1 Compare October 8, 2020 04:54

aitikgupta requested review from eric-wieser and rossbar October 8, 2020 04:56

mattip closed this Oct 8, 2020

mattip reopened this Oct 8, 2020

rossbar previously requested changes Oct 8, 2020

View reviewed changes

aitikgupta force-pushed the unwanted-mode-dep branch from 7708fe1 to b3f86f9 Compare October 8, 2020 16:53

aitikgupta requested a review from rossbar October 8, 2020 16:55

aitikgupta force-pushed the unwanted-mode-dep branch from b3f86f9 to 4dbc544 Compare October 9, 2020 13:42

eric-wieser reviewed Oct 9, 2020

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

eric-wieser reviewed Oct 9, 2020

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

aitikgupta force-pushed the unwanted-mode-dep branch 2 times, most recently from 9cf6fb9 to e811a8f Compare October 9, 2020 14:27

eric-wieser reviewed Oct 9, 2020

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

aitikgupta force-pushed the unwanted-mode-dep branch from e811a8f to 0cc1285 Compare October 10, 2020 04:42

eric-wieser reviewed Oct 10, 2020

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

aitikgupta force-pushed the unwanted-mode-dep branch from 04cb219 to 6e417cb Compare October 10, 2020 10:01

eric-wieser reviewed Oct 10, 2020

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

aitikgupta force-pushed the unwanted-mode-dep branch from 6e417cb to 5fe981e Compare October 10, 2020 12:34

aitikgupta changed the title ~~DEP: Deprecate inexact matches for mode in np.correlate and np.convolve~~ DEP: Shift correlate mode parsing to C and deprecate inexact matches Oct 22, 2020

aitikgupta requested a review from eric-wieser October 22, 2020 22:45

eric-wieser reviewed Jan 19, 2021

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

eric-wieser approved these changes Jan 19, 2021

View reviewed changes

eric-wieser reviewed Jan 19, 2021

View reviewed changes

doc/release/upcoming_changes/17492.deprecation.rst Outdated Show resolved Hide resolved

eric-wieser reviewed Jan 19, 2021

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved

eric-wieser reviewed Jan 19, 2021

View reviewed changes

numpy/core/tests/test_numeric.py Outdated Show resolved Hide resolved

Base automatically changed from master to main March 4, 2021 02:05

aitikgupta and others added 8 commits March 18, 2021 20:48

DEP: Deprecate inexact matches of mode, shift parameter parsing to C

f53eaf7

TST: Added test for inexact matches of mode

6256db8

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

35158c7

REL: Add deprecation release note

4bcfc9d

Remove extra information

153c8ed

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

Add one more test for illegal arguments

f5a2b48

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

The same test for np.correlate

edfd440

A few whitespace cleanups

89de3d9

mattip force-pushed the unwanted-mode-dep branch from 80def6f to 89de3d9 Compare March 18, 2021 19:03

aitikgupta commented Mar 18, 2021

View reviewed changes

numpy/core/src/multiarray/conversion_utils.c Show resolved Hide resolved

mattip merged commit ddbff08 into numpy:main Mar 18, 2021

	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);
	}

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

	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")

Uh oh!

Conversation

aitikgupta commented Oct 7, 2020

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattip commented Oct 8, 2020

Uh oh!

rossbar left a comment

Choose a reason for hiding this comment

Uh oh!

aitikgupta commented Oct 8, 2020

Uh oh!

eric-wieser commented Oct 8, 2020

Uh oh!

aitikgupta commented Oct 8, 2020

Uh oh!

eric-wieser commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aitikgupta commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aitikgupta commented Oct 9, 2020

Uh oh!

Uh oh!

Uh oh!

eric-wieser Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitikgupta Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

eric-wieser Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

aitikgupta Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aitikgupta commented Oct 12, 2020

Uh oh!

aitikgupta commented Nov 4, 2020

Uh oh!

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

aitikgupta commented Jan 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aitikgupta commented Jan 19, 2021

Uh oh!

mattip commented Feb 6, 2021

Uh oh!

mattip commented Mar 18, 2021

Uh oh!

eric-wieser commented Mar 18, 2021

eric-wieser commented Oct 8, 2020 •

edited

Loading

aitikgupta commented Oct 9, 2020 •

edited

Loading

eric-wieser left a comment •

edited

Loading

eric-wieser Oct 10, 2020 •

edited

Loading

aitikgupta Oct 10, 2020 •

edited

Loading