Skip to content

TST Common test preserve dtype #18054

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 31 commits into from
Sep 8, 2020
Merged

TST Common test preserve dtype #18054

merged 31 commits into from
Sep 8, 2020

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Aug 1, 2020 •
edited by rth
Loading

closes #16290
helps toward #11000

Take over #16290 and introduce a common test to check if estimators preserve dtypes

glemaitre
glemaitre commented Aug 1, 2020
View reviewed changes
thomasjpfan
thomasjpfan reviewed Aug 7, 2020
View reviewed changes
sklearn/utils/estimator_checks.py Outdated
Comment on lines 212 to 215
# it's not possible to preserve dtypes in transform with clustering
# same for MissingIndicator
if (
not isinstance(transformer, (ClusterMixin, MissingIndicator))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled with a preserves_dtype tag that is empty?

@glemaitre glemaitre changed the title Common test preserve dtype [WIP] Common test preserve dtype Aug 12, 2020
@glemaitre glemaitre force-pushed the common_test_preserve_dtype branch from afb17fa to 43af513 Compare August 12, 2020 13:53
@glemaitre glemaitre changed the title [WIP] Common test preserve dtype TST Common test preserve dtype Aug 13, 2020
rth
rth reviewed Aug 13, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. Minor comments otherwise LGTM.

@glemaitre
Copy link
Member Author

To solve the remaining failure, we should first merge: #18045

rth
rth approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks LGTM, pending #18045 being merged to fix the last failing test.

@rth rth mentioned this pull request Aug 15, 2020
Merged
@glemaitre
Copy link
Member Author

@rth my bad, I did a typo in the tag name and so the test was run. It should pass now

@rth
Copy link
Member

rth commented Aug 19, 2020

Thanks! +1 for me. @thomasjpfan do you have any other comments?

thomasjpfan
thomasjpfan reviewed Aug 22, 2020
View reviewed changes
sklearn/utils/estimator_checks.py Outdated
Comment on lines 1523 to 1525
X_trans,
X_trans_float_64,
atol=1e-10, rtol=5e-4,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have PRs where we had to adjust these tolerances for different environments. Are we okay with running into this issue in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, this part of the test has 2 issues:

  • precision issue as you mentioned;
  • dtype issue (if np.float32 is the only supported dtype).

However, we caught a bug using this. So it is kind of useful.

To solve the first issue, I think that we could design a decorator that could adjust the tolerance depending on the platform. For the second case, I think that we should avoid this testing if np.float64 is not the default dtype.

thomasjpfan
thomasjpfan reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run this with git tag [arm64] to see if this passes there?

After this gets merged, it would be interesting to see this run on conda-forge to see if it passes there as well.

NicolasHug
NicolasHug reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick pass, mostly looks good

@rth
Copy link
Member

rth commented Sep 7, 2020

Any other comments @NicolasHug @thomasjpfan ?

thomasjpfan
thomasjpfan approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glemaitre glemaitre merged commit 21569e7 into scikit-learn:master Sep 8, 2020
@glemaitre
Copy link
Member Author

3 approvals, I am going to merge :)
Thanks @ksslng for the original PR.

@agramfort
Copy link
Member

agramfort commented Sep 8, 2020 via email

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@glemaitre @ksslng
@rth @jayzed82
TST Common test preserve dtype (scikit-learn#18054)
f90a13c 
Co-authored-by: Sebastian Kießling <sebkies@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug left review comments

@agramfort agramfort agramfort approved these changes

@rth rth rth approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:decomposition module:preprocessing module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@glemaitre @rth @agramfort @NicolasHug @thomasjpfan @ksslng