Skip to content
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.

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

[MRG] Doc: Fix dbscan #10662

Merged
merged 2 commits into from Feb 23, 2018
Merged

[MRG] Doc: Fix dbscan #10662

merged 2 commits into from Feb 23, 2018

Conversation

wdevazelhes
Copy link
Contributor

Changes for the already merged PR #10646 according to review #10646 (review)

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

Thanks @wdevazelhes

@@ -43,7 +43,7 @@ def dbscan(X, eps=0.5, min_samples=5, metric='minkowski', metric_params=None,
metric : string, or callable
The metric to use when calculating distance between instances in a
feature array. If metric is a string or callable, it must be one of
the options allowed by ``metrics.pairwise.pairwise_distances`` for its
the options allowed by :func:`metrics.pairwise_distances` for its
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 it'll work unless we use :func:`sklearn.metrics.pairwise.pairwise_distances` or something like :func:`...<...>`

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure classes.rst lists it in the proposed form. I would check the circle artefacts, but that is harder on my phone

Copy link
Member

Choose a reason for hiding this comment

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

Not an expert of these sphinx details, but that does not seem to work. Look at https://19070-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.cluster.dbscan.html for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it does not seem to work. To choose between :func:`sklearn.metrics.pairwise.pairwise_distances` or :func:`...<...>` I ran a git grep:
git grep -P ':(mod|func|meth):`.*pairwise.*' and we see that the full name seem to be more used. What is more, looking at the 3rd result of git grep ( doc/glossary.rst: :func:`metrics.pairwise_kernels`. These can compute pairwise distance) we see in the doc version that @jnothman 's version works there (http://scikit-learn.org/dev/glossary.html#term-pairwise-metrics). However we also notice that the link just after to metrics.pairwise_kernelsdoes not work. For now maybe I could just push the full version in the PR to make it work but then in another PR/issue, deal with the fact that we cannot reference by metrics.pairwise.pairwise_distances from here, but that we can in the glossary ? And also another PR/issue to deal with metrics.pairwise_kernels's case ?

Output of git grep -P ':(mod|func|meth):`.*pairwise.*' :

doc/glossary.rst:        (disregarding :mod:`metrics.pairwise`), as distinct from the
doc/glossary.rst:                :func:`fit` and similar methods consists of pairwise measures over
doc/glossary.rst:        Cosine Distance) through :func:`metrics.pairwise_distances`, and of
doc/glossary.rst:        :func:`metrics.pairwise_kernels`.  These can compute pairwise distance
doc/glossary.rst:        two data points.  See :func:`metrics.pairwise_distances`.  In practice,
doc/modules/clustering.rst:    in the :mod:`sklearn.metrics.pairwise` module.
doc/modules/gaussian_process.rst:All Gaussian process kernels are interoperable with :mod:`sklearn.metrics.pairwise`
doc/modules/gaussian_process.rst:``metric`` to pairwise_kernels`` from :mod:`sklearn.metrics.pairwise`. Moreover,
doc/modules/metrics.rst:The :mod:`sklearn.metrics.pairwise` submodule implements utilities to evaluate
doc/modules/neighbors.rst:brute-force algorithm based on routines in :mod:`sklearn.metrics.pairwise`.
doc/modules/neighbors.rst:routines available in :mod:`sklearn.metrics.pairwise`.
doc/whats_new/older_versions.rst:- Added n_jobs option to :func:`metrics.pairwise.pairwise_distances`
doc/whats_new/older_versions.rst:  and :func:`metrics.pairwise.pairwise_kernels` for parallel computation,
doc/whats_new/older_versions.rst:- Distance helper functions :func:`metrics.pairwise.pairwise_distances`
doc/whats_new/older_versions.rst:  and :func:`metrics.pairwise.pairwise_kernels` by Robert Layton
doc/whats_new/v0.15.rst:- Added :func:`metrics.pairwise_distances_argmin_min`, by Philippe Gervais.
doc/whats_new/v0.16.rst:- Parallelized calculation of :func:`pairwise_distances` is now supported
doc/whats_new/v0.16.rst:  formerly approximated :func:`rbf_kernel <metrics.pairwise.rbf_kernel>`
doc/whats_new/v0.17.rst:  :func:`sklearn.metrics.pairwise.cosine_similarity`. By
doc/whats_new/v0.17.rst:- Added :func:`metrics.pairwise.laplacian_kernel`.  By `Clyde Fare <https://github.com/Clyde-fare>`_.
doc/whats_new/v0.17.rst:- Added the ``X_norm_squared`` parameter to :func:`metrics.pairwise.euclidean_distances`
doc/whats_new/v0.18.rst:- Fixed a bug where :func:`metrics.pairwise.cosine_distances` could return a
doc/whats_new/v0.18.rst:- :func:`metrics.pairwise.pairwise_distances` now converts arrays to
doc/whats_new/v0.19.rst:  :func:`metrics.pairwise.pairwise_kernels` :issue:`5211` by
sklearn/cluster/dbscan_.py:        the options allowed by :func:`metrics.pairwise_distances` for its
sklearn/cluster/dbscan_.py:        the options allowed by :func:`metrics.pairwise_distances` for its
sklearn/metrics/cluster/unsupervised.py:        allowed by :func:`metrics.pairwise.pairwise_distances
sklearn/metrics/cluster/unsupervised.py:        allowed by :func:`sklearn.metrics.pairwise.pairwise_distances`. If X is
sklearn/neighbors/lof.py:        :func:`sklearn.metrics.pairwise.pairwise_distances`. When p = 1, this

Copy link
Member

@lesteve lesteve Feb 23, 2018

Choose a reason for hiding this comment

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

Thanks a lot for look in details into this. My advice: just use the fully qualified name and move on with your life. IMO there are a lot more interesting things to do that fighting sphinx quirks ;-). I just pushed a tweak doing this and I'll merge after I double-check the generated documentation.

I think the currentmodule directive is what makes it work in the glossary. I am not sure whether you can use currentmodule for docstrings and even then it may be not be worth it to complicate docstrings with sphinx-specific things.

@lesteve lesteve merged commit 0e04f1c into scikit-learn:master Feb 23, 2018
@lesteve
Copy link
Member

lesteve commented Feb 23, 2018

Merging, thanks a lot @wdevazelhes!

For the record, I reused your git grep and figured out that in .py files all the pairwise_distances are fully qualified:

❯ git grep -P ':(mod|func|meth):`.*pairwise.*' -- '*.py'
sklearn/cluster/dbscan_.py:        the options allowed by :func:`sklearn.metrics.pairwise_distances` for
sklearn/cluster/dbscan_.py:        the options allowed by :func:`sklearn.metrics.pairwise_distances` for
sklearn/metrics/cluster/unsupervised.py:        allowed by :func:`metrics.pairwise.pairwise_distances
sklearn/metrics/cluster/unsupervised.py:        allowed by :func:`sklearn.metrics.pairwise.pairwise_distances`. If X is
sklearn/neighbors/lof.py:        :func:`sklearn.metrics.pairwise.pairwise_distances`. When p = 1, this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants