Skip to content

Conversation

@betatim
Copy link
Member

@betatim betatim commented Sep 2, 2022

Reference Issues/PRs

contributes to #11000

What does this implement/fix? Explain your changes.

This makes it so that the projection used in RBFSampler uses float32 when the input X is float32, otherwise it uses float64 as before.

Any other comments?

One inline comment about what to do with the docstring.

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. 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.
@betatim
Copy link
Member Author

betatim commented Sep 2, 2022

The change in tutorial.rst is bigger than this PR. It used to state that input is always cast to float64 but that is no longer true. I think a better change to the tutorial would be to mention the rule (of thumb) that lets a user know if or if not the dtype will be changed. However, I don't know if there is a rule like that.

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)?

@ogrisel
Copy link
Member

ogrisel commented Sep 2, 2022

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.

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.

Still LGTM. Thanks for the latest fixes/improvements.

@ogrisel ogrisel added Waiting for Reviewer Quick Review For PRs that are quick to review labels Sep 2, 2022
@glemaitre glemaitre self-requested a review September 5, 2022 15:40
Copy link
Member

@glemaitre glemaitre left a 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.

@glemaitre
Copy link
Member

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]>
@jeremiedbb
Copy link
Member

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.

And another test checking that the transformed arrays from float32 and float64 inputs are close up to some tolerance (from sklearn.utils._testing import assert_allclose).

@betatim
Copy link
Member Author

betatim commented Sep 7, 2022

I've added a test to check that the attributes are float32/float64 and that the results are allclose. Not sure if the allclose test is meaningful though, ideas for more realistic Xs to use?

Copy link
Member

@jeremiedbb jeremiedbb left a 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.

Copy link
Member

@glemaitre glemaitre left a 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.

@betatim
Copy link
Member Author

betatim commented Sep 9, 2022

Changed to global dtype and applied review suggestions.

I think this is ready to roll :D

Thanks for all the help and comments!

@glemaitre glemaitre merged commit 92f555e into scikit-learn:main Sep 9, 2022
@betatim betatim deleted the dtype-RBFSampler branch September 9, 2022 15:15
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Sep 12, 2022
Co-authored-by: Olivier Grisel <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Quick Review For PRs that are quick to review Waiting for Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants