MAINT Convert int to intp_t ctype def in tree/ related code
#27546
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>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
int to intp_t ctype defint to intp_t ctype def in tree/ related code
int to intp_t ctype def in tree/ related codeint to intp_t ctype def in tree/ related code
Signed-off-by: Adam Li <adam2392@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
|
Just a small gentle follow-up. @glemaitre: do you think we can merge this PR? |
glemaitre
reviewed
Jan 17, 2024
There was a problem hiding this comment.
I made a pass. I think it makes sense to use intp_t whenever we have variable linked to indexing and sizing. A lot of changes are related to return value that are usually 0, 1, -1 because it is related to failure. In general, those are always int by convention and I don't think it make sense to do something different.
| @@ -1383,6 +1383,7 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| (self.n_trees_per_iteration_, grid.shape[0]), dtype=Y_DTYPE | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
There was a problem hiding this comment.
Can you update the docstring above to specify the dtype of grid and target_features.
| @@ -1061,6 +1061,8 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| (n_trees_per_stage, grid.shape[0]), dtype=np.float64, order="C" | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
|
|
|||
There was a problem hiding this comment.
You can specify the dtype of grid and target_features in the docstring as well.
| @@ -1403,6 +1403,7 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| shape=grid.shape[0], dtype=np.float64, order="C" | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
There was a problem hiding this comment.
you can also update the grid and target_features dtype in the docstring.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
adam2392
commented
Jan 17, 2024
| @@ -1383,6 +1383,7 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| (self.n_trees_per_iteration_, grid.shape[0]), dtype=Y_DTYPE | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
| @@ -1061,6 +1061,8 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| (n_trees_per_stage, grid.shape[0]), dtype=np.float64, order="C" | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
|
|
|||
| @@ -1403,6 +1403,7 @@ def _compute_partial_dependence_recursion(self, grid, target_features): | |||
| averaged_predictions = np.zeros( | |||
| shape=grid.shape[0], dtype=np.float64, order="C" | |||
| ) | |||
| target_features = np.asarray(target_features, dtype=np.intp, order="C") | |||
sklearn/tree/_tree.pyx
Outdated
Comment on lines
66
to
69
| cdef intp_t IS_FIRST = 1 | ||
| cdef intp_t IS_NOT_FIRST = 0 | ||
| cdef intp_t IS_LEFT = 1 | ||
| cdef intp_t IS_NOT_LEFT = 0 |
There was a problem hiding this comment.
Sure that makes sense. I changed
glemaitre
approved these changes
Jan 22, 2024
|
Enabling auto-merge. |
5 tasks
glemaitre
added a commit
to glemaitre/scikit-learn
that referenced
this pull request
Feb 10, 2024
…ikit-learn#27546) Signed-off-by: Adam Li <adam2392@gmail.com> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@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
Follow-on to #27352 and #27539
Related to #25572
What does this implement/fix? Explain your changes.
Converts
inttointp_twhere applicableAny other comments?
LOC such as:
in
splitter.pyxis not convertible as it is used in alibcstdlib function, which seems to only acceptint?