MAINT Fix ctypedef types in tree submodule #27352
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
jjerphan
reviewed
Oct 4, 2023
Comment on lines
+35
to
+39
| cdef double weighted_n_samples # Weighted number of samples (in total) | ||
| cdef double weighted_n_node_samples # Weighted number of samples in the node | ||
| cdef double weighted_n_left # Weighted number of samples in the left node | ||
| cdef double weighted_n_right # Weighted number of samples in the right node | ||
| cdef double weighted_n_missing # Weighted number of samples that are missing |
There was a problem hiding this comment.
Could double (here and also elsewhere) be changed to float64_t?
There was a problem hiding this comment.
Sure, I can make the change in this PR, or do it in a follow-on? I could do the int there as well as @lorentzenchr suggested below.
WDYT?
There was a problem hiding this comment.
A separate PR would be easier to review.
There was a problem hiding this comment.
In that case, I'll leave this as is for now. Would this PR be mergable? I can help test and make the systematic change in the tree submodule once we resolve this PR?
lorentzenchr
reviewed
Oct 5, 2023
Comment on lines
+35
to
+39
| cdef double weighted_n_samples # Weighted number of samples (in total) | ||
| cdef double weighted_n_node_samples # Weighted number of samples in the node | ||
| cdef double weighted_n_left # Weighted number of samples in the left node | ||
| cdef double weighted_n_right # Weighted number of samples in the right node | ||
| cdef double weighted_n_missing # Weighted number of samples that are missing |
| @@ -554,24 +554,24 @@ cdef inline int node_split_best( | |||
|
|
|||
| # Sort n-element arrays pointed to by feature_values and samples, simultaneously, | |||
| # by the values in feature_values. Algorithm: Introsort (Musser, SP&E, 1997). | |||
| cdef inline void sort(DTYPE_t* feature_values, SIZE_t* samples, SIZE_t n) noexcept nogil: | |||
| cdef inline void sort(float32_t* feature_values, intp_t* samples, intp_t n) noexcept nogil: | |||
| if n == 0: | |||
| return | |||
| cdef int maxd = 2 * <int>log(n) | |||
There was a problem hiding this comment.
Suggested change
| cdef int maxd = 2 * <int>log(n) | |
| cdef intp_t maxd = 2 * <intp_t>log(n) |
| cdef DTYPE_t pivot | ||
| cdef SIZE_t i, l, r | ||
| cdef void introsort(float32_t* feature_values, intp_t *samples, | ||
| intp_t n, int maxd) noexcept nogil: |
There was a problem hiding this comment.
Suggested change
| intp_t n, int maxd) noexcept nogil: | |
| intp_t n, intp_t maxd) noexcept nogil: |
lorentzenchr
approved these changes
Oct 5, 2023
jjerphan
approved these changes
Oct 5, 2023
|
I would treat the replacement for |
This was referenced Oct 6, 2023
REDVM
pushed a commit
to REDVM/scikit-learn
that referenced
this pull request
Nov 16, 2023
Signed-off-by: Adam Li <adam2392@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Related to #25572
What does this implement/fix? Explain your changes.
I was playing around with ctypedefs and saw the related GH issue, which is a very nice cleanup. I went ahead and tried it and the code seems to be able to compile and pass unit-tests w/o issue.
This has to implement a change across all the tree submodule simultaneously and the
_gradient_boosting.pyxfile since they all rely on the types defined in thesklearn/tree/_tree.pxdfile.If you think there is a simpler strategy that refactors less LOC, then lmk.
Any other comments?
Hope this helps.