-
Notifications
You must be signed in to change notification settings - Fork 6
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
Bump Cython Old #18
Bump Cython Old #18
Conversation
since support for usage of DEF/IF is expected to be removed, IF are remove from code as well.
cyarray/msstdint.h was added in 24c49cc since stdint.h has not been there pre MSVC2015. It should be fine to remove this now.
d0c464b
to
c482445
Compare
cython>3.0 disallows setting `ndarray.data=<char *>some_data`. This commit introduces a dirty workaround for this.
- all nogil prefixed with noexcept
c482445
to
58dd4ad
Compare
ef6a1d3
to
f157c4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me apart from the change to the pyproject.toml
which I am not sure about.
@@ -2,7 +2,7 @@ | |||
requires = [ | |||
"wheel>=0.29.0", | |||
"setuptools>=42.0.0", | |||
"oldest-supported-numpy", | |||
"Cython<3.0", | |||
"numpy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it recommended to use "numpy" instead of "oldest-supported-numpy" now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if that is recommended, but the problem is this:
requirements.txt and setup.py have just "numpy". So, pip install -r requirements.txt
installs the latest numpy.
Then with pip install -e .
cyarray is compiled with oldest-supported-numpy as specified in pyproject.toml.
Eventually it ends up with,
ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject
like here: https://github.com/pypr/cyarray/actions/runs/10434451995/job/28897324886
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so what is the oldest-supported-numpy version here? Is it < 2.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, its 1.23.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor nit, apart from that I am happy to merge this. Thanks!
cyarray/carray.pyx.mako
Outdated
|
||
cimport numpy as np | ||
np.import_array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the semicolon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have amended this and force pushed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
pyproject.toml specified oldest supported numpy. This is changed. The .pyx file is also updated to avoid avoid errors.
f157c4e
to
c7c2068
Compare
Old fix from https://github.com/rainwoodman/pandas/blob/05d3fe2402e4563124e7060837ded7513ab5bca7/pandas/_libs/reduction.pyx#L27