8000 MAINT float 32bit support for `DistanceMetric` by jjerphan · Pull Request #22764 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT float 32bit support for DistanceMetric #22764

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 21 commits into from
May 30, 2022
Merged

MAINT float 32bit support for DistanceMetric #22764

merged 21 commits into from
May 30, 2022

Conversation

jjerphan
Copy link
Member
@jjerphan jjerphan commented Mar 11, 2022 •
edited
Loading

Reference Issues/PRs

Extract of #22590.
Precedes #22590.

What does this implement/fix? Explain your changes.

This extends the set of the current DistanceMetric to introduce a set of DistanceMetric32 via Tempita to support pairs of 32bit vectors.

This also removes commented code of classes which were not distances.

@jjerphan
MAINT 32bit support for DistanceMetric
2f56a49 
This has been extracted from 7645ba.
@jjerphan jjerphan marked this pull request as ready for review March 30, 2022 15:57
ogrisel
ogrisel approved these changes Apr 4, 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.

Some nitpicks but otherwise looks good to me!

jjerphan and others added 3 commits April 4, 2022 17:45
@jjerphan @ogrisel
Apply suggestions from code review
afe246b 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan @ogrisel
TST Use assert_allclose
b80f475 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan
DOC Fix formatting and mathematics
f26dd48
< 10000 div id="pullrequestreview-933690819" class="js-comment js-updatable-content js-socket-channel js-targetable-element js-minimize-container" data-gid="PRR_kwDOAAzd1s43pv3D" data-channel="eyJjIjoicHVsbF9yZXF1ZXN0X3Jldmlldzo5MzM2OTA4MTkiLCJ0IjoxNzQ3NTk4NzI2fQ==--ccd2e6e6443d66c7b23ad175fc17991431e19f1e558827f87684367f57d59232" data-url="/scikit-learn/scikit-learn/pull/22764/partials/reviews/933690819" >
jjerphan
jjerphan commented Apr 6, 2022
View reviewed changes
@jjerphan jjerphan changed the title MAINT 32bit support for DistanceMetric MAINT float 32bit support for DistanceMetric Apr 13, 2022
@jjerphan jjerphan force-pushed the maint/distance-metrics-32bit branch from 771f3bb to 51ae02e Compare April 29, 2022 12:08
@glemaitre glemaitre self-requested a review April 29, 2022 15:37
ogrisel
ogrisel approved these changes Apr 29, 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.

Still LGTM overall but some of my previous comments seems to have been ignored.

jjerphan and others added 4 commits April 29, 2022 20:03
@jjerphan @ogrisel
Reword comment
1b532ff 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan @ogrisel
MAINT Remove bitness
868992e 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan
Merge branch 'main' into maint/distance-metrics-32bit
2c3bdb0
@jjerphan
Post merge fix
20bdcb6
@jjerphan jjerphan mentioned this pull request May 25, 2022
Merged
jeremiedbb
jeremiedbb reviewed May 25, 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.

Looks good. Just a few typos and a couple of comments. I also think that we should have a test that checks that the distance matrix computed from X32 and Y32 is close to the one computed from X64 and Y64.

jjerphan and others added 5 commits May 26, 2022 09:31
@jjerphan @jeremiedbb
Apply review comments
e37cb23 
Also add a few changes.

Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jjerphan
TST Add dtype-agnosticity test for DtanceMetrics
120be0e
@jjerphan
Merge branch 'main' into maint/distance-metrics-32bit
6811705
@jjerphan @jeremiedbb
TST Revert error messages
B41A a906a4d 
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jjerphan
fixup! TST Revert error messages
ec13cad
@jjerphan jjerphan mentioned this pull request May 30, 2022
Closed
3 tasks
jeremiedbb
jeremiedbb approved these changes May 30, 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. Thanks @jjerphan !

@jeremiedbb jeremiedbb merged commit 48a0fe6 into scikit-learn:main May 30, 2022
@jjerphan jjerphan deleted the maint/distance-metrics-32bit branch May 30, 2022 15:19
ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@jjerphan @ogrisel @jeremiedbb
MAINT float32 support for DistanceMetric (scikit-learn#22764)
48874f4 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
@jjerphan jjerphan mentioned this pull request May 26, 2023
Merged
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

@glemaitre glemaitre Awaiting requested review from glemaitre

Assignees
No one assigned
Labels
cython 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