Skip to content

Cython v3 support #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

munechika-koyo
Copy link
Contributor

@munechika-koyo munechika-koyo commented Jul 28, 2023

Hello,

I tried to resolve cython v3 incompatibilities by adding reversed binary arithmetic operators like __radd__, __rmul__, etc.
Existing unit test has been passed, but I did not make sure that they cover all of methods.
I would appreciate it if you would check & comment on this PR.

Copy link
Contributor

@vsnever vsnever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @munechika-koyo,

I have looked at the code and I think the new binary methods are implemented correctly. Plus, all the tests and demos I've run are working fine.

Cython currently generates tons of warnings about deprecated DEF statements. However, I think that so far there is no adequate replacement for them. Although many of these statements can be replaced with global cdef variables, this does not work for those that define the size of static arrays, like here:

DEF NN = 312
DEF MM = 156
# The array for the state vector
cdef uint64_t mt[NN]

There is a draft PR with a proposal to add global constants (e.g. cdef const double CONST), cython/cython#5242. I think that until this or similar functionality is implemented, DEF statements in Raysect or Cherab should remain.

Also, Cython 3 allows to get rid of that annoying deprecated NumPy-1.7 C-API warning. This can be done by defining the macro in setup.py:

macros = [("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")]
...
                    extensions.append(Extension(module, [pyx_file], include_dirs=compilation_includes, extra_compile_args=compilation_args, define_macros=macros),)
...
                    extensions.append(Extension(module, [c_file], include_dirs=compilation_includes, extra_compile_args=compilation_args, define_macros=macros),)

@munechika-koyo
Copy link
Contributor Author

munechika-koyo commented Aug 2, 2023

Thank you for confirming my PR! @vsnever

There is a draft PR with a proposal to add global constants (e.g. cdef const double CONST), cython/cython#5242. I think that until this or similar functionality is implemented, DEF statements in Raysect or Cherab should remain.

Yes, I agree with you. We should follow this issue and check at which version DEF statement will be depricated.

Also, Cython 3 allows to get rid of that annoying deprecated NumPy-1.7 C-API warning. This can be done by defining the macro in setup.py:

I added some commits according to your recommendation. Please confirm it as well.

Copy link
Contributor

@vsnever vsnever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @munechika-koyo.
Please note that my approval only means that I agree to these changes as a user. Only @CnlPepper decides what changes will be in the Raysect code.

@jacklovell
Copy link
Contributor

jacklovell commented Oct 20, 2023

Cython currently generates tons of warnings about deprecated DEF statements. However, I think that so far there is no adequate replacement for them. Although many of these statements can be replaced with global cdef variables, this does not work for those that define the size of static arrays, like here:

It is possible to have compile time constant integers in Cython without DEF, by using an anonymous enum. Your example would become:

cdef enum:
    NN = 312 
    MM = 156 
  
 # The array for the state vector 
 cdef uint64_t mt[NN] 

Is it clearer than a bunch of DEF statements? I'm not convinced. Is it nicer than being spammed with warnings about DEF being deprecated? Probably.

It would at least solve this specific case of defining the size of statically-sized arrays, as well as a bunch of other integer definitions in the code. The DEF INFINITY instances can be replaced by from math cimport INFINITY. There are also a lot of places in the code where DEF is used for floats which will need a different solution: none of them quite fit as math cimports.

@vsnever
Copy link
Contributor

vsnever commented Oct 20, 2023

It would at least solve this specific case of defining the size of statically-sized arrays, as well as a bunch of other integer definitions in the code. The DEF INFINITY instances can be replaced by from math cimport INFINITY. There are also a lot of places in the code where DEF is used for floats which will need a different solution: none of them quite fit as math cimports.

It looks like the Cython developers have removed the deprecation warnings for DEF statements for now, cython/cython#4310 (comment). While switching to enum for integers is ok, I'd wait to see if the Cython developers come up with solution for compile-time constants that replaces DEF.

Copy link
Contributor

@jacklovell jacklovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks OK to me. As well as a few inline comments, I'd suggest the following:

  • Please update pyproject.toml to specify cython~=3.0 in the build requirements.
  • Please remove emojis from your commit messages: they don't render properly in the pager git uses by default on many OSs.

Tests all pass without issue, but there are some build warnings when building with Cython 3.0.11. There are lots of "performance hints" such as:

performance hint: raysect/core/math/cython/interpolation/linear.pxd:38:20: No exception value declared for 'linear2d' in pxd file.
Users cimporting this function and calling it without the gil will always require an exception check.
Suggest adding an explicit exception value.

And

performance hint: raysect/core/math/statsarray.pyx:707:5: Exception check on '_add_sample' will always require the GIL to be acquired.
Possible solutions:
        1. Declare '_add_sample' as 'noexcept' if you control the definition and you're sure you don't want the function to raise exce
ptions.
        2. Use an 'int' return type on '_add_sample' to allow an error code to be returned.

Personally I don't think these are a big issue at the moment, since Raysect is used with multiprocessing rather than multithreading, and cleaning them all up would drastically increase the scope of this PR. But maybe worth addressing in a subsequent PR, especially if the Python 3.13 free-threading build is going to be getting more attention in future.

There's also a number of NumPy API warnings, e.g.:

raysect/optical/spectrum.c: In function ‘__pyx_f_7raysect_7optical_8spectrum_8Spectrum__construct’:
raysect/optical/spectrum.c:21871:3: warning: passing argument 1 of ‘PyArray_DATA’ from incompatible pointer type [enabled by default]
   PyArray_FILLWBYTE(__pyx_t_1, 0);
   ^
In file included from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarrayobject.h:18:0,
                 from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/arrayobject.h:4,
                 from raysect/optical/spectrum.c:1259:
/home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarraytypes.h:1470:1: note: expected ‘struct PyArrayObject *’ but argument is of type ‘struct PyObject *’
 PyArray_DATA(PyArrayObject *arr)
 ^
raysect/optical/spectrum.c:21871:3: warning: passing argument 1 of ‘PyArray_ITEMSIZE’ from incompatible pointer type [enabled by default]
   PyArray_FILLWBYTE(__pyx_t_1, 0);
   ^
In file included from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarrayobject.h:18:0,
                 from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/arrayobject.h:4,
                 from raysect/optical/spectrum.c:1259:
/home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarraytypes.h:1524:1: note: expected ‘const struct PyArrayObject *’ but argument is of type ‘struct PyObject *’
 PyArray_ITEMSIZE(const PyArrayObject *arr)
 ^
raysect/optical/spectrum.c:21871:3: warning: passing argument 1 of ‘PyArray_DIMS’ from incompatible pointer type [enabled by default]
   PyArray_FILLWBYTE(__pyx_t_1, 0);
   ^
In file included from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarrayobject.h:18:0,
                 from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/arrayobject.h:4,
                 from raysect/optical/spectrum.c:1259:
/home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarraytypes.h:1482:1: note: expected ‘struct PyArrayObject *’ but argument is of type ‘struct PyObject *’
 PyArray_DIMS(PyArrayObject *arr)
 ^
raysect/optical/spectrum.c:21871:3: warning: passing argument 1 of ‘PyArray_NDIM’ from incompatible pointer type [enabled by default]
   PyArray_FILLWBYTE(__pyx_t_1, 0);
   ^
In file included from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarrayobject.h:18:0,
                 from /home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/arrayobject.h:4,
                 from raysect/optical/spectrum.c:1259:
/home/jlovell/venvs/raysect-dev/lib/python3.7/site-packages/numpy/core/include/numpy/ndarraytypes.h:1464:1: note: expected ‘const struct PyArrayObject *’ but argument is of type ‘struct PyObject *’
 PyArray_NDIM(const PyArrayObject *arr)
 ^

It's unclear to me if this is a Cython bug or a Raysect bug: the Raysect code in the pyx file looks OK to me, and the warnings aren't produced when building master (using Cython 0.29.37 and without defining NPY_NO_DEPRECATED_API).


else:

return NotImplemented

# TODO - add 2D affine transformations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this TODO?

@vsnever
Copy link
Contributor

vsnever commented Oct 31, 2024

It's unclear to me if this is a Cython bug or a Raysect bug: the Raysect code in the pyx file looks OK to me, and the warnings aren't produced when building master (using Cython 0.29.37 and without defining NPY_NO_DEPRECATED_API).

Just checked the Cython output, Cython correctly casts pointer types following the definitions in the NumPy array API. It looks like this was a NumPy bug because I no longer get these warnings with NumPy 2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants