Skip to content

MAINT Replace double with float64_t inside tree submodule #27539

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 4 commits into from
Oct 6, 2023

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Oct 6, 2023

Reference Issues/PRs

Follow-up to #27352 (comment)

Related to: #25572

What does this implement/fix? Explain your changes.

Replaces double with float64_t inside tree submodule.

I can also replace int with intp_t in the same PR if desirable.

Any other comments?

n/a

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392
Copy link
Member Author

adam2392 commented Oct 6, 2023

lmk if you prefer me to do int -> intp_t here or in a downstream PR

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

✔️ Linting Passed

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

Generated for commit: 42754b5. Link to the linter CI: here

Signed-off-by: Adam Li <adam2392@gmail.com>
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.

About intp_t, I think it would require a bit more careful reviewing. So better do it in a follow-up PR.

Comment on lines +214 to +217
cdef float64_t left_child_min
cdef float64_t left_child_max
cdef float64_t right_child_min
cdef float64_t right_child_max
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Those were not defined before. I looked into _tree.cpp and obviously Cython figures out to define them as double variables.

@lorentzenchr lorentzenchr changed the title [MAINT] Replace double with float64_t inside tree/ submodule MAINT Replace double with float64_t inside tree submodule Oct 6, 2023
@lorentzenchr lorentzenchr merged commit 2105500 into scikit-learn:main Oct 6, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
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.

3 participants