10000 TST Add test for quasi equality for `PairwiseDistancesReduction` results by jjerphan · Pull Request #23490 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

TST Add test for quasi equality for PairwiseDistancesReduction results #23490

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 18 commits into from
Jun 9, 2022
Merged

TST Add test for quasi equality for PairwiseDistancesReduction results #23490

merged 18 commits into from
Jun 9, 2022

Conversation

jjerphan
Copy link
Member
@jjerphan jjerphan commented May 30, 2022 •
edited
Loading

Reference Issues/PRs

Partly extracted from #22590

What does this implement/fix? Explain your changes.

Modify tests to add quasi-equality for 32bit implementations.

This comes by anticipation.

Any other comments?

@jjerphan
TST Add test for quasi equa
85ae9a4 
This comes before porting implementations toits.
@jjerphan jjerphan changed the title TST Add test for quasi equality TST Add test for quasi equality for PairwiseDistancesReduction results May 30, 2022
@ogrisel
Copy link
Member
ogrisel commented May 30, 2022

Could you please add a test to check that assert_argkmin_results_quasi_equality is working correctly on some fixed data crafted on purpose?

@jjerphan jjerphan mentioned this pull request May 30, 2022
Closed
3 tasks
@jjerphan jjerphan marked this pull request as ready for review June 1, 2022 10:04
@jjerphan jjerphan added Waiting for Reviewer float32 Issues related to support for 32bit data labels Jun 1, 2022
@jjerphan @ogrisel
TST Inline values in assertion and complete comments
57ae169 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan and others added 2 commits June 7, 2022 17:31
@jjerphan @ogrisel
TST Complete assertion with radius
b1ef2f5 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan @ogrisel
TST Rename variables
5d4fb18 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan and others added 2 commits June 7, 2022 17:59
@jjerphan @ogrisel
Improve comments
10000
15bcf62 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan
TST Improve some more
d9013e9
ogrisel
ogrisel approved these changes Jun 7, 2022
View reviewed changes
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.

Another pass of reviews, otherwise LGTM!

@jjerphan @ogrisel
TST Apply review comments
93496ae 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan @jeremiedbb @ogrisel
Apply review comments
589ecae 
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan @ogrisel
Improve readability
badc58e 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
ogrisel
ogrisel approved these changes Jun 8, 2022
View reviewed changes
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.

Final pass of review. This LGTM!

@ogrisel
Copy link
Member
ogrisel commented Jun 8, 2022

Actually there is now a bunch of TypeError: assert_radius_neighborhood_results_equality() missing 1 required positional argument: 'radius' in the tests.

@jjerphan @ogrisel
TST Review comments
8e2f82a 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jeremiedbb
jeremiedbb approved these changes Jun 9, 2022
View reviewed changes
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

@jjerphan @jeremiedbb
Use rtol directly
a412ac8 
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jeremiedbb jeremiedbb merged commit 8b4191c into scikit-learn:main Jun 9, 2022
@jeremiedbb
Copy link
Member

Thanks @jjerphan

ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@jjerphan @ogrisel @jeremiedbb
TST Add test for quasi equality for PairwiseDistancesReduction resu…
66f206a 
…lts (scikit-learn#23490)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jjerphan jjerphan deleted the tst/pairwise_distances_reductions-quasi-equality branch October 21, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
float32 Issues related to support for 32bit data module:metrics No Changelog Needed Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jjerphan @ogrisel @jeremiedbb
0