-
-
Notifications
You must be signed in to change notification settings - Fork 26.5k
ENH Use X's dtype for the projection in RBFSampler
#24317
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
Conversation
ogrisel
left a comment
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.
LGTM. Can you please just fix the broken doctest in the tutorial and add an entry to document the change in the changelog (doc/whats_new/v1.2.rst).
The tutorial mentions that input is always cast to flaot64, which is no longer true.
|
The change in Can/should we add an example where the dtype is changed (in addition to the one that is there now where the dtype is preserved)? |
I don't think so because over time those remaining float64 only estimators might disappear and we don't want to have to find a new example each time we fix the one we used as an example in the tutorial. |
Co-authored-by: Olivier Grisel <[email protected]>
ogrisel
left a comment
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.
Still LGTM. Thanks for the latest fixes/improvements.
glemaitre
left a comment
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.
Otherwise it looks good.
|
I think that it is also worth adding a test to check that the fitted attributes are also 32 bits floating-point. This would not be covered by the common test. |
Co-authored-by: Guillaume Lemaitre <[email protected]>
And another test checking that the transformed arrays from float32 and float64 inputs are close up to some tolerance ( |
|
I've added a test to check that the attributes are float32/float64 and that the results are |
jeremiedbb
left a comment
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.
LGTM. Just a couple of nitpicks.
glemaitre
left a comment
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.
LGTM once the global_dtype change is done.
Co-authored-by: Guillaume Lemaitre <[email protected]>
|
Changed to global dtype and applied review suggestions. I think this is ready to roll :D Thanks for all the help and comments! |
Co-authored-by: Olivier Grisel <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]>
Reference Issues/PRs
contributes to #11000
What does this implement/fix? Explain your changes.
This makes it so that the projection used in
RBFSamplerusesfloat32when the inputXisfloat32, otherwise it usesfloat64as before.Any other comments?
One inline comment about what to do with the docstring.