-
Notifications
You must be signed in to change notification settings - Fork 661
Several ensemble scikit-learn compatibility improvements
#8023
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
Changes from all commits
ecdf895
a57e413
971e268
9b37325
bb440d8
37595cc
8f98440
9855936
137be01
7e67063
1e69ba8
2bd2395
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -408,14 +408,15 @@ class BaseRandomForestModel(Base, InteropMixin): | |
| layout=layout, | ||
| default_chunk_size=default_chunk_size, | ||
| align_bytes=align_bytes, | ||
| ensure_all_finite=True, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick question for the accel path here, with ensure_all_finite=True getting passed to ForestInference from _predict_model_on_gpu, and the proxy's _check_inputs already running check_array(..., ensure_all_finite=True) on X to detect NaN, aren't we now traversing X twice during accel predict? Once to translate NaN -> UnsupportedOnGPU, once inside FIL. Another small thing probably, but just wanted to ask about it more than block on it
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed we are in |
||
| ) | ||
|
|
||
| def _fit_forest(self, X, y): | ||
| cdef bool is_classifier = self._estimator_type == "classifier" | ||
| cdef bool is_float32 = X.dtype == np.float32 | ||
|
|
||
| cdef uintptr_t X_ptr = X.ptr | ||
| cdef uintptr_t y_ptr = y.ptr | ||
| cdef uintptr_t X_ptr = X.data.ptr | ||
| cdef uintptr_t y_ptr = y.data.ptr | ||
| cdef int n_rows = X.shape[0] | ||
| cdef int n_cols = X.shape[1] | ||
| cdef level_enum verbose = <level_enum> self._verbose_level | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we control this error so this is fine, just gave me a bit of paus thinking about an obscure bug if we change say to "nan" instead of "NaN" or another change. Probably not worth mulling much about, but still gave me a bit of pause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't love it, but it was quick to do and there's tests that will start failing if behavior ever changes. Seems better than nothing.