Skip to content

MNT Use cimport numpy as cnp for sklearn/tree #23315

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 14, 2022
Merged

MNT Use cimport numpy as cnp for sklearn/tree #23315

merged 6 commits into from
May 14, 2022

Conversation

RMeli
Copy link
Contributor

@RMeli RMeli commented May 9, 2022

Reference Issues/PRs

Addresses #23295 (sklearn/tree)

What does this implement/fix? Explain your changes.

Change np to cnp to reference NumPy's C API according to issue #23295 for sklearn/tree/*.

thomasjpfan
thomasjpfan reviewed May 10, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan 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 for the PR!

I think there are some more in sklearn/tree/_tree.pyx?

@RMeli
Copy link
Contributor Author

RMeli commented May 10, 2022

Indeed, and quite a lot of them. Thank you for spotting this @thomasjpfan !

thomasjpfan
thomasjpfan approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan 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 for the update! LGTM

@glemaitre glemaitre self-requested a review May 13, 2022 11:50
glemaitre
glemaitre reviewed May 13, 2022
View reviewed changes
glemaitre
glemaitre reviewed May 13, 2022
View reviewed changes
glemaitre
glemaitre approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

thomasjpfan
thomasjpfan reviewed May 14, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

There are some merge conflicts in the splitter to resolve.

@RMeli @thomasjpfan
Update sklearn/tree/_splitter.pyx
07dbb0d 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@thomasjpfan thomasjpfan merged commit 3850935 into scikit-learn:main May 14, 2022
@alfaro96 alfaro96 mentioned this pull request May 17, 2022
Closed
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
@RMeli @glemaitre @thomasjpfan
MNT Use cimport numpy as cnp for sklearn/tree (scikit-learn#23315)
483e2a9 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
glemaitre added a commit that referenced this pull request Aug 5, 2022
@RMeli @glemaitre @thomasjpfan
MNT Use cimport numpy as cnp for sklearn/tree (#23315)
9de3577 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
cython module:tree
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RMeli @thomasjpfan @glemaitre