MAINT: resolve UPDATEIFCOPY deprecation errors #8150
Conversation
|
This should be ready to go. It will start working right after the API version is bumped. |
|
See here for the passing tests: https://travis-ci.org/scipy/scipy/jobs/301100231 |
There was a problem hiding this comment.
My review simply suggests cleaning up the C preprocessor code a little bit.
It may also be helpful to briefly summarize why the current Travis test matrix can't pass with this PR if there's some strange reason for that (I understand there's some tricky low level stuff going on so maybe just a quick note on that so we know it is ok or not).
scipy/ndimage/src/nd_image.c
Outdated
| @@ -190,6 +194,9 @@ static PyObject *Py_Correlate1D(PyObject *obj, PyObject *args) | |||
|
|
|||
| NI_Correlate1D(input, weights, axis, output, (NI_ExtendMode)mode, cval, | |||
| origin); | |||
| #if NPY_API_VERSION >= 0x0000000c | |||
| PyArray_ResolveWritebackIfCopy(output); | |||
| #endif | |||
There was a problem hiding this comment.
This C preprocessor if / endif block is used quite a lot in this PR in different places. Could it perhaps be helpful from a maintenance / code duplication standpoint to abstract the preprocessor code to a single macro declaration at the top of the C module, or perhaps even better in one of the associated header files that is included?
There was a problem hiding this comment.
Yes, HAVE_WRITEBACKIFCOPY or similar please
|
The Travis-CI is not passing because of numpy/numpy#10014. There is no action required in SciPy. |
| #if NPY_API_VERSION >= 0x0000000c | ||
| #define HAVE_WRITEBACKIFCOPY | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I don't think this really changes much. Rather, I think the idea is to define a macro here. Probably something like this:
#define HAVE_WRITEBACKIFCOPY(output) if ( NPY_API_VERSION >= 0x0000000c ) \
PyArray_ResolveWritebackIfCopy(output);And then just a single HAVE_WRITEBACKIFCOPY(output) statement should suffice at the various locations where there are the ifdef blocks I think. Just have to be careful with the preprocessor literal substitutions (my syntax might be slightly off).
There was a problem hiding this comment.
That's not going to work, as the symbol will not be resolved at compile-time.
|
I think the ifdefs are ok, better not try to abstract it away too much.
|
|
scipy/sparse/sparsetools/sparsetools.cxx also needs similar change.
The correct place for writeback is here
https://github.com/scipy/scipy/blob/master/scipy/sparse/sparsetools/sparsetools.cxx#L444
along the lines of `if (is_output[j] && arg_arrays[j] != NULL) {
PyArray_WriteBackIfCopy(arg_arrays[j]); }` before the XDECREF.
|
|
@pv Done. |
ghost
closed this
ghost
reopened this
ghost
closed this
ghost
reopened this
|
Hopefully that will fix the stupid typos that I made. |
There was a problem hiding this comment.
Ok, the full CI suite is passing & I see that upstream numpy fixes have been applied as well. I'm +1 to merge as long as @pv is happy with the preprocessor adjustments (looks like his comments were addressed).
|
Thanks, merged. Code style nit: typically |
ghost
deleted the
I'm aware of that, and it's very frustrating. When I'm reading C code, I always have to spend an extra few seconds matching the preprocessor definitions. Who came up with this rule? |
ghost
mentioned this pull request
rgommers
added
the
backport-candidate
|
backporting this to 1.0.x, it fixes a lot of test errors there |
(cherry picked from commit 73f723a)
(cherry picked from commit 73f723a)
rgommers
removed
the
backport-candidate
Closes #8145.