-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT remove unnecessary cython.import_array #28656
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
MNT remove unnecessary cython.import_array #28656
Conversation
Just curious, is it mostly clean-up or does this have an effect in practice? Since we are going to move to Meson as our main build backend #28506 hopefully soon, how important do you think this is? Do you think the equivalent is needed in |
@lesteve I also updated the meson files, except in one place where it loops over files and the numpy include does not really cost anything. What costs, is the import_array as in makes the *.c file larger. I have to say, I was surprised how rarely we really need import_array, as we are mainly using memoryviews instead of cnp.array. |
Great if you updated the |
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 tried to build this PR with meson locally in editable mode and everything looks fine. The size of the generated ".so" files seems mostly unchanged unfortunately (I had expected some reduction).
My main comment is below, otherwise, LGTM.
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.
Thank you, @lorentzenchr.
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.
LGTM.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Alright, let's auto-merge when green. Thanks all! |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Reference Issues/PRs
Towards #25572
What does this implement/fix? Explain your changes.
This PR removes the cython
cnp.import_array()
statement and the numpy includes where not necessary.Any other comments?
Whe include numpy headers way more often than needed. For instance, modules
linear
,ensemble._hist_gradient_boosting
andsvm
don't need it.This PR got a littler bigger than anticipated 😏