Skip to content

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

Merged
merged 8 commits into from
Mar 20, 2024

Conversation

lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Mar 18, 2024

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 and svm don't need it.
This PR got a littler bigger than anticipated 😏

Copy link

github-actions bot commented Mar 18, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: f9a8215. Link to the linter CI: here

@lesteve
Copy link
Member

lesteve commented Mar 19, 2024

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 meson.build files? IIRC I mostly try to mirror setup.py files when writing meson.build files but I may have tweaked a few things in a few places.

@lorentzenchr
Copy link
Member Author

@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.

@lesteve
Copy link
Member

lesteve commented Mar 19, 2024

Great if you updated the meson.build files, I missed this somehow!

Copy link
Member

@ogrisel ogrisel left a 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.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @lorentzenchr.

Copy link
Member

@ogrisel ogrisel left a 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>
@ogrisel ogrisel enabled auto-merge (squash) March 20, 2024 17:23
@ogrisel
Copy link
Member

ogrisel commented Mar 20, 2024

Alright, let's auto-merge when green. Thanks all!

@ogrisel ogrisel merged commit 59bba8f into scikit-learn:main Mar 20, 2024
23 checks passed
@ogrisel ogrisel deleted the mnt_remove_cnp_import_array branch March 21, 2024 09:32
jpienaar-tuks pushed a commit to jpienaar-tuks/scikit-learn that referenced this pull request Mar 23, 2024
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants