[CI fix] add import_array in cython files where numpy is cimport-ed #4592
Conversation
|
Thanks for looking into this @emanuelle I can confirm that the same issue occurs in PyWavelets and adding I also ran into some failures similar to the remaining ones seen here. Specifically, the errors stating
|
accidentally marked approved while still debugging
|
Thanks a lot for the tip @grlee77 I would have spent a lot of time today trying to understand this |
|
@grlee77 since you already took a look, the failures in th
8000
e denoise tests are indeed related to this fused types bug which was corrected in Cython master but the colormixer failing tests seem to be a different kind of problem. It's a |
I was not familiar with this one, but did take a quick look. The following change (based on this Stack Overflow question) to the test class, does get it to work for me (the change is just the addition of class TestColorMixerAdd(ColorMixerTest):
op = staticmethod(cm.add)
py_op = np.add
positive = 50
positive_clip = 56
negative = -50
negative_clip = -220
class TestColorMixerMul(ColorMixerTest):
op = staticmethod(cm.multiply)
py_op = np.multiply
positive = 1.2
positive_clip = 2
negative = 0.5
negative_clip = -0.5 |
|
I can open a Cython issue related to the change in class attribute/bound method behavior. update: see cython/cython#3529 |
|
thank you so much @grlee77 ! |
|
An additional benefit of adding these |
| def _nl_means_denoising_2d(cnp.ndarray[np_floats, ndim=3] image, Py_ssize_t s=7, | ||
| Py_ssize_t d=13, double h=0.1, double var=0.): | ||
| def _nl_means_denoising_2d(cnp.ndarray[np_floats, ndim=3] image, Py_ssize_t s, | ||
| Py_ssize_t d, double h, double var): |
There was a problem hiding this comment.
it's ok to remove default values here since the defaults are given in the Python function and passed to the Cython one.
|
Just about "removing default values" - this looks to be working around a bug that we think we've fixed. It might be worth waiting for Cython alpha 2? (unless the default values really aren't needed, or it's really important to get the CI to build it as soon as possible) |
|
@da-woods indeed we should remove the default values as soon as alpha 2 is out but I prefer to have the CI green, it's quite scary for contributors to see all these failed jobs, and also we become less attentive to other potential CI failures (thinking yeah the CI fails these days...). The workaround does not change at all the behaviour of the function. |
There was a problem hiding this comment.
I approve. Removing the defaults in _nl_means helper functions is fine because as @emmanuelle says, these are hidden helper functions called from pure Python wrappers which preserve the user-facing defaults. Good to know we could use that motif in the future, but they were never necessary in these Cython helper functions.
|
🎉 !!! |
Closes #4577 (hopefully).
Lots of places where
import_arrayshould be added, but let's check first whether the CI is green.