Skip to content

FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes #26754

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 6 commits into from
Jul 6, 2023
Merged

FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes #26754

merged 6 commits into from
Jul 6, 2023

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Jul 3, 2023

Reference Issues/PRs

Fixes #26742.

What does this implement/fix? Explain your changes.

#25482 made {Ball,KD}Tree.valid_metrics private and wrongly created an eponymous class-method. This is a wrong API breaking change.

This PR revert parts of #25482 and adaptions introduced in HDBSCAN to make {Ball,KD}Tree.valid_metrics public class attributes.

It also documents {Ball,KD}Tree.valid_metrics.

Any other comments?

@jjerphan
FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes
a913f51 
Also document them.

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan jjerphan mentioned this pull request Jul 3, 2023
Closed
@github-actions
Copy link

github-actions bot commented Jul 3, 2023 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 1420513. Link to the linter CI: here

@jjerphan
black: Format sklearn/cluster/_hdbscan/hdbscan.py
73b1d35 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan
test: Better document test
f6055bd 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan jjerphan modified the milestones: 1.3.1, 1.3 Jul 3, 2023
ogrisel
ogrisel approved these changes Jul 3, 2023
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.

Thanks for the fix. We also need an entry in the changelog for 1.3.1.

sklearn/neighbors/tests/test_neighbors_tree.py Outdated
Non-regression test for #26742"""

valid_metrics_list = BinaryTree.valid_metrics
assert all(isinstance(valid_metric, str) for valid_metric in valid_metrics_list)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need such a test. This attribute is already tested by other code, tests and doctest.

And the fact that it is public API is already stated by the fact that there is no leading underscore in the attribute name, the class name and the module name, and the fact that it is documented in doc/modules/neighbors.rst.

jjerphan and others added 2 commits July 3, 2023 17:37
@jjerphan @ogrisel
Remove test
26fd4d6 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@jjerphan
DOC Add section for 1.3.1 and an entry for this PR
0860e79 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jeremiedbb
Copy link
Member

What was the motivation for making it a class method ? Reading the original PR I understood that it was for rendering reasons in the docs. Going back to a property is fine ?

@jjerphan
Copy link
Member Author

jjerphan commented Jul 3, 2023 •
edited
Loading

What was the motivation for making it a class method ? Reading the original PR I understood that it was for rendering reasons in the docs.

That was the motivation.

Going back to a property is fine ?

Property only are usable on instances (so that's not possible). Note that this PR documents valid_metrics as a public attribute, so the original intend of #25482 is preserved.

@jjerphan
[scipy-dev] CI Test against the latest SciPy and Numpy version
1420513 
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jeremiedbb
Copy link
Member

Property only are usable on instances (so that's not possible)

sorry I meant class attribute (that is as it was before). Anyway the rendering looks good KDTree.valid_metrics (no lower case). Thanks

@jeremiedbb
Copy link
Member

There are compilation issues with scipy-dev that are being fixed in #26771. It's not relevant to this PR, let's merge.

@jeremiedbb jeremiedbb merged commit d7605c4 into scikit-learn:main Jul 6, 2023
@jjerphan jjerphan deleted the fix/revert-valid_metrics-to-public-class-attributes branch July 6, 2023 11:56
@lucianolorenti lucianolorenti mentioned this pull request Jul 23, 2023
Closed
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
@jjerphan @ogrisel @punndcoder28
FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes (s…
6c8dd3f 
…cikit-learn#26754)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@jjerphan @ogrisel @glemaitre
FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes (s…
8c570b3 
…cikit-learn#26754)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
@jjerphan @ogrisel @jeremiedbb
FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes (#…
2206eca 
…26754)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@danibene danibene mentioned this pull request Sep 21, 2023
Closed
@ChengjieLi28 ChengjieLi28 mentioned this pull request Sep 22, 2023
Merged
2 tasks
@stamate stamate mentioned this pull request Nov 2, 2023
Merged
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@jjerphan @ogrisel @REDVM
FIX Revert {Ball,KD}Tree.valid_metrics to public class attributes (s…
8f73dff 
…cikit-learn#26754)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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
cython module:cluster module:neighbors No Changelog Needed
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.

KDtree.valid_metrics is no longer an attribute without warning
3 participants
@jjerphan @jeremiedbb @ogrisel