10000 [MRG + 2 -.5] Listed valid metrics for neighbors algorithms by vinayak-mehta · Pull Request #4525 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 2 -.5] Listed valid metrics for neighbors algorithms #4525

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
Closed
wants to merge 2 commits into from
Closed

[MRG + 2 -.5] Listed valid metrics for neighbors algorithms #4525

wants to merge 2 commits into from

Conversation

vinayak-mehta
Copy link
Contributor

@vinayak-mehta
Copy link
Contributor Author

@amueller - Is this what you meant by:

There is also _VALID_METRICS in the metrics module which lists the metrics for the non-trees.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 6fa67a7 on vortex-ape:list_valid_metrics into 9206a78 on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.15% when pulling 3926ed2 on vortex-ape:list_valid_metrics into 9206a78 on scikit-learn:master.

amueller
amueller reviewed Apr 5, 2015
View reviewed changes
doc/modules/neighbors.rst
@@ -427,6 +482,29 @@ and the ``'effective_metric_'`` is in the ``'VALID_METRICS'`` list of
same order as the number of training points, and that ``leaf_size`` is
close to its default value of ``30``.

Valid Metrics for Nearest Neighbor Algorithms
---------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a doctests that shows the valid_metrics class attributes?

Copy link
Member

Choose a reason for hiding this comment

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

This shows the attributes to the users, but also makes sure that a doctests fails in this place if someone modifies the valid metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how to do that :\ (doctest showing valid_metrics class attributes) and searching didn't help. Can you point me to an example?

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 3926ed2 on vortex-ape:list_valid_metrics into 9206a78 on scikit-learn:master.

@amueller
Copy link
Member
amueller commented Apr 5, 2015

similar to http://scikit-learn.org/dev/modules/model_evaluation.html#common-cases-predefined-values
only you need to just print(KDTree.valid_metrics)

vinayak-mehta
vinayak-mehta reviewed Apr 5, 2015
View reviewed changes
doc/modules/neighbors.rst
'seuclidean', 'sokalsneath', 'kulsinski', 'minkowski',
'mahalanobis', 'p', 'l2', 'hamming', 'l1', 'wminkowski', 'pyfunc'
======================== =================================================================

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like it has been done in the Usage examples under that table in model_selection? So I should do something like this here?

>>> from sklearn.neighbors import KDTree
>>> print(KDTree.valid_metrics)

Sorry for being noob.

Copy link
Member

Choose a reason for hiding this comment

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

yes, and then list the output.

@vinayak-mehta
Copy link
Contributor Author

Ah! I just understood what you meant with:

Can you also add a doctests that shows the valid_metrics class attributes?
This shows the attributes to the users, but also makes sure that a doctests fails in this place if
someone modifies the valid metrics.

I had done this in #4356, was confusing what you asked with checking the output of print(KDTree.valid_metrics) with the valid metrics I mentioned for KDTree in the table :| I guess I needed some sleep last night. :P

vinayak-mehta
vinayak-mehta reviewed Apr 6, 2015
View reviewed changes
doc/modules/neighbors.rst Outdated

>>> from sklearn.neighbors import KDTree
>>> print(KDTree.valid_metrics) # doctest: +ELLIPSIS
[...]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add ... as print was giving different output everytime :| Not sure how it will fail if someone modifies the valid metrics.

Copy link
Member

Choose a reason for hiding this comment

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

Diffferent output between different versions? How could it be very different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different outputs like this. this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For every run.

Copy link
Member

Choose a reason for hiding this comment

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

print(sort(KDTree.valid_metrics))?

@vinayak-mehta vinayak-mehta force-pushed the list_valid_metrics branch 3 times, most recently from 22a8610 to 52e513e Compare April 10, 2015 23:36
@vinayak-mehta vinayak-mehta changed the title Listed valid metrics for neighbors algorithms [MRG] Listed valid metrics for neighbors algorithms Apr 21, 2015
@vinayak-mehta
Copy link
Contributor Author

@amueller - Should any more changes be added?

@amueller
Copy link
Member

LGTM

@amueller amueller changed the title [MRG] Listed valid metrics for neighbors algorithms [MRG + 1] Listed valid metrics for neighbors algorithms Apr 22, 2015
@vinayak-mehta vinayak-mehta deleted the list_valid_metrics branch September 11, 2015 06:26
@amueller
Copy link
Member

why did you close?

@vinayak-mehta
Copy link
Contributor Author

Oops, sorry :\ I was deleting unused branches, must have deleted it by mistake :|

@vinayak-mehta vinayak-mehta restored the list_valid_metrics branch September 11, 2015 17:08
@vinayak-mehta
Copy link
Contributor Author

Is there any way I can undo this?

@vinayak-mehta vinayak-mehta reopened this Sep 11, 2015
@raghavrv
Copy link
Member

Now rebase it!! :)

@vinayak-mehta
Copy link
Contributor Author

Yeah, fetching from upstream.

amueller
amueller reviewed Sep 11, 2015
View reviewed changes
doc/modules/neighbors.rst
@@ -252,6 +252,62 @@ the lower half of those faces.
multi-output regression using nearest neighbors.


Nearest Centroid Classifier
Copy link
Member

Choose a reason for hiding this comment

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

Is this now in the right place? There is no deletions...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Yes, as per #4521 (comment) its in the right place above the details on the trees and neighbors implementations.

@jmschrei
Copy link
Member

This LGTM as well.

@vinayak-mehta
Copy link
Contributor Author

@raghavrv Yes, will do.

@vinayak-mehta
Listed valid metrics in neighbors.rst
368dad0 
Reordered algorithms

Added doctest

Sorted metrics

Changed example wording
@vinayak-mehta
Reordered algorithms (after fixing merge conflicts)
686a256
vinayak-mehta
vinayak-mehta commented Nov 28, 2016
View reviewed changes
doc/modules/neighbors.rst

>>> from sklearn.neighbors import KDTree
>>> import numpy as np
>>> print(np.sort(KDTree.valid_metrics)) # doctest: +ELLIPSIS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raghavrv I don't think we need numpy for this doctest, a normal sorted would work just fine?

Copy link
Member

Choose a reason for hiding this comment

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

indeed!!

@vinayak-mehta
Copy link
Contributor Author

@raghavrv Apart from my comment, I think this is ready to merge.

@raghavrv
Copy link
Member

Thanks @vinayak-mehta! I'll review this in a while...

@raghavrv raghavrv added this to the 0.18.2 milestone Nov 28, 2016
jnothman
jnothman requested changes Nov 28, 2016
View reviewed changes
doc/modules/neighbors.rst
'russellrao', 'seuclidean', 'sokalmichener',
'sokalsneath', 'sqeuclidean', 'yule', 'wminkowski'

**K-D Tree** 'chebyshev', 'euclidean', 'cityblock', 'manhattan', 'infinity',
Copy link
Member

Choose a reason for hiding this comment

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

We should note that modules/generated/sklearn.neighbors.DistanceMetric.html lists the measures for K-D and Ball Trees, along with their arguments. Or else we should just be pointing to that reference here rather than listing measures, so as to avoid duplication.

@raghavrv raghavrv modified the milestones: 0.18.2, 0.19 Nov 30, 2016
@jnothman
Copy link
Member

This received a LGTM from @amueller and @jmschrei. However, I'd rather referring the reader to modules/generated/sklearn.neighbors.DistanceMetric.html instead of listing the measures here. WDYT?

@jnothman jnothman changed the title [MRG + 1] Listed valid metrics for neighbors algorithms [MRG + 2 -.5] Listed valid metrics for neighbors algorithms Jun 14, 2017
@amueller
Copy link
Member

@jnothman I might not have been aware of that docstring. I'd be fine with a very obvious pointer towards it. Having this listed in the user guide seems helpful but duplication is not great.

@amueller amueller closed this Jun 19, 2017
@amueller amueller reopened this Jun 19, 2017
@amueller
Copy link
Member

(misclick sorry)

@cmarmo cmarmo removed this from the 0.19 milestone Jun 13, 2020
Base automatically changed from master to main January 22, 2021 10:48
@hongshaoyang hongshaoyang mentioned this pull request Feb 6, 2021
Merged
@hongshaoyang
Copy link
Contributor

Opened #19379 to take over this.

@cmarmo cmarmo removed the help wanted label Feb 6, 2021
@vinayak-mehta vinayak-mehta deleted the list_valid_metrics branch June 9, 2021 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raghavrv raghavrv raghavrv left review comments

@jnothman jnothman jnothman requested changes

Assignees
No one assigned
Labels
Documentation Stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vinayak-mehta @landscape-bot @coveralls @amueller @raghavrv @jmschrei @jnothman @hongshaoyang @cmarmo
0