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 + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order #9206

Merged
merged 77 commits into from Jul 11, 2017

Conversation

Projects
None yet
5 participants
@raghavrv
Member

raghavrv commented Jun 23, 2017

Taking over Alex's #9023

Fixes #7758

Also refer alternative implementation at #7793

What does this implement/fix? Explain your changes.

This adds a unit test to check that all parameters are documented and in the right order.

cc: @agramfort @neerajgangwar

@raghavrv

This comment has been minimized.

Show comment
Hide comment
@raghavrv

raghavrv Jun 23, 2017

Member

These need fixing - EDIT: Fixed

E           sklearn.covariance.outlier_detection.__init__ arg mismatch: ['random_state'] (EllipticEnvelope)
E           sklearn.decomposition.fastica_.transform copy != y (FastICA)
E           sklearn.decomposition.fastica_.transform y != copy (FastICA)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params']
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (ElasticNet)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (ElasticNetCV)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (Lasso)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskElasticNet)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskElasticNetCV)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskLasso)
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params']
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params'] (LassoCV)
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params'] (MultiTaskLassoCV)
E           sklearn.preprocessing.data.inverse_transform arg mismatch: ['X'] (QuantileTransformer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (Binarizer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (Normalizer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (StandardScaler)
Member

raghavrv commented Jun 23, 2017

These need fixing - EDIT: Fixed

E           sklearn.covariance.outlier_detection.__init__ arg mismatch: ['random_state'] (EllipticEnvelope)
E           sklearn.decomposition.fastica_.transform copy != y (FastICA)
E           sklearn.decomposition.fastica_.transform y != copy (FastICA)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params']
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (ElasticNet)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (ElasticNetCV)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (Lasso)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskElasticNet)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskElasticNetCV)
E           sklearn.linear_model.coordinate_descent.enet_path arg mismatch: ['params'] (MultiTaskLasso)
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params']
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params'] (LassoCV)
E           sklearn.linear_model.coordinate_descent.lasso_path arg mismatch: ['params'] (MultiTaskLassoCV)
E           sklearn.preprocessing.data.inverse_transform arg mismatch: ['X'] (QuantileTransformer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (Binarizer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (Normalizer)
E           sklearn.preprocessing.data.transform arg mismatch: ['copy'] (StandardScaler)

raghavrv added some commits Jul 6, 2017

@jnothman

jnothman approved these changes Jul 7, 2017 edited

I think this will be good to have merged. I haven't noticed specific issues, but if the tests are passing, that's a pretty good thing. We've had numerous attempts at this and it would be good to have it settled.

The main thing I wonder is why this is not in numpydoc, and I hope it is soon (though the existence of "Other parameters" in some projects makes it trickier).

The other thing I'd like to see along these lines is that we test consistency between our docs with something like:

def assert_consistent_docs(objects,
                           include_params=None, exclude_params=None,
                           include_attribs=None, exclude_attribs=None
                           include_returns=None, exclude_returns=None):
    """

    Checks if types and descriptions of parameters, etc, are identical across
    objects. ``object``s may either be ``NumpyDocString`` instances or objects
    (classes, functions, descriptors) with docstrings that can be parsed as
    numpydoc.

    By default it asserts that any Parameters/Returns/Attributes entries having the
    same name among ``objects`` docstrings also have the same type
    specification and description (ignoring whitespace).

    ``include_*`` and ``exclude_*`` parameters here are mutually exclusive,
    and specify a whitelist or blacklist, respectively, of parameter, attribute or
    return value names.
    """
    pass

def assert_param_docstring_matches(objects, section, param_name, regex):
    # handles cases like random_state that should have more-or-less the same description everywhere
callback :
Callable that gets invoked every five iterations.
callback : callable or None, optional (default: None)
callable that gets invoked every five iterations

This comment has been minimized.

@jnothman

jnothman Jul 7, 2017

Member

You lost the capitalisation here. I wonder what the signature of the callable is.

@jnothman

jnothman Jul 7, 2017

Member

You lost the capitalisation here. I wonder what the signature of the callable is.

Show outdated Hide outdated sklearn/utils/testing.py
Show outdated Hide outdated sklearn/utils/testing.py
@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jul 7, 2017

Member

good enough for me

+1 for MRG

Member

agramfort commented Jul 7, 2017

good enough for me

+1 for MRG

@agramfort agramfort changed the title from [MRG + 1] Add a check to test the docstring params and their order to [MRG + 1 (rv) + 1 (alex)] Add a check to test the docstring params and their order Jul 7, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 8, 2017

Member

I'd like my nitpick regard naming addressed. Otherwise, happy for this to be merged (without thorough review on my part)

Member

jnothman commented Jul 8, 2017

I'd like my nitpick regard naming addressed. Otherwise, happy for this to be merged (without thorough review on my part)

@jnothman

The thing that's missing here (but can be in a future PR) is advice to developers (for the main project and for contrib). In general, we have some updates to issue contrib (including changing dependency versions).

Otherwise, LGTM on the basis of a fairly superficial review.

@jnothman jnothman changed the title from [MRG + 1 (rv) + 1 (alex)] Add a check to test the docstring params and their order to [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order Jul 9, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 9, 2017

Member

flake8

./sklearn/tests/test_docstring_parameters.py:19:1: F401 'sklearn.utils.testing._get_func_name' imported but unused
from sklearn.utils.testing import _get_func_name
^
./sklearn/tests/test_docstring_parameters.py:127:21: F821 undefined name 'get_func_name'
            name_ = get_func_name(func)
                    ^
./sklearn/utils/testing.py:852:17: F821 undefined name 'get_func_name'
    func_name = get_func_name(func, class_name=class_name)
                ^
Member

jnothman commented Jul 9, 2017

flake8

./sklearn/tests/test_docstring_parameters.py:19:1: F401 'sklearn.utils.testing._get_func_name' imported but unused
from sklearn.utils.testing import _get_func_name
^
./sklearn/tests/test_docstring_parameters.py:127:21: F821 undefined name 'get_func_name'
            name_ = get_func_name(func)
                    ^
./sklearn/utils/testing.py:852:17: F821 undefined name 'get_func_name'
    func_name = get_func_name(func, class_name=class_name)
                ^
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 9, 2017

Member

Does that flake mean something isn't being run??

Member

jnothman commented Jul 9, 2017

Does that flake mean something isn't being run??

@@ -312,6 +312,9 @@ def linkage_tree(X, connectivity=None, n_components=None,
be symmetric and only the upper triangular half is used.
Default is None, i.e, the Ward algorithm is unstructured.
n_components : int (optional)

This comment has been minimized.

@jnothman

jnothman Jul 9, 2017

Member

This parameter is unused. See #9307

@jnothman

jnothman Jul 9, 2017

Member

This parameter is unused. See #9307

This comment has been minimized.

@raghavrv

raghavrv Jul 10, 2017

Member

(You don't want this addressed in this PR right?)

@raghavrv

raghavrv Jul 10, 2017

Member

(You don't want this addressed in this PR right?)

@raghavrv

This comment has been minimized.

Show comment
Hide comment
@raghavrv

raghavrv Jul 10, 2017

Member

Let's merge this if you are happy? @jnothman @agramfort

Member

raghavrv commented Jul 10, 2017

Let's merge this if you are happy? @jnothman @agramfort

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jul 10, 2017

Member

any idea why appveyor is not happy?

Member

agramfort commented Jul 10, 2017

any idea why appveyor is not happy?

@raghavrv

This comment has been minimized.

Show comment
Hide comment
@raghavrv

raghavrv Jul 10, 2017

Member

That seems to be because of #9317. should be fixed after that is merged and I update master. Or you can do it separately too. It should not break anything.

Member

raghavrv commented Jul 10, 2017

That seems to be because of #9317. should be fixed after that is merged and I update master. Or you can do it separately too. It should not break anything.

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jul 10, 2017

Member
Member

agramfort commented Jul 10, 2017

@raghavrv

This comment has been minimized.

Show comment
Hide comment
@raghavrv

raghavrv Jul 11, 2017

Member

I think it's safe to merge without waiting for appveyor @agramfort (more delay leads to merge conflicts. especially after sprints)

Member

raghavrv commented Jul 11, 2017

I think it's safe to merge without waiting for appveyor @agramfort (more delay leads to merge conflicts. especially after sprints)

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jul 11, 2017

Member

all green !

thx @raghavrv

Member

agramfort commented Jul 11, 2017

all green !

thx @raghavrv

@agramfort agramfort merged commit b6f8865 into scikit-learn:master Jul 11, 2017

4 of 5 checks passed

codecov/patch 43.26% of diff hit (target 96.36%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 96.15% (-0.22%) compared to 90f0bbf
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@raghavrv raghavrv deleted the raghavrv:test_docstring branch Jul 11, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jul 11, 2017

Member
Member

jnothman commented Jul 11, 2017

massich added a commit to massich/scikit-learn that referenced this pull request Jul 13, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge tag '0.19b2' into releases
Release 0.19b2

* tag '0.19b2': (808 commits)
  Preparing 0.19b2
  [MRG+1] FIX out of bounds array access in SAGA (#9376)
  FIX make test_importances pass on 32 bit linux
  Release 0.19b1
  DOC remove 'in dev' header in whats_new.rst
  DOC typos in whats_news.rst [ci skip]
  [MRG] DOC cleaning up what's new for 0.19 (#9252)
  FIX t-SNE memory usage and many other optimizer issues (#9032)
  FIX broken link in gallery and bad title rendering
  [MRG] DOC Replace \acute by prime (#9332)
  Fix typos (#9320)
  [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (#9206)
  DOC Residual sum vs. regression sum (#9314)
  [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (#9317)
  More informative error message for classification metrics given regression output (#9275)
  [MRG] COSMIT Remove unused parameters in private functions (#9310)
  [MRG+1] Ridgecv normalize (#9302)
  [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (#7388)
  Add data_home parameter to fetch_kddcup99 (#9289)
  FIX makedirs(..., exists_ok) not available in Python 2 (#9284)
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'releases' into dfsg (reremoved joblib and jquery)
* releases: (808 commits)
  Preparing 0.19b2
  [MRG+1] FIX out of bounds array access in SAGA (#9376)
  FIX make test_importances pass on 32 bit linux
  Release 0.19b1
  DOC remove 'in dev' header in whats_new.rst
  DOC typos in whats_news.rst [ci skip]
  [MRG] DOC cleaning up what's new for 0.19 (#9252)
  FIX t-SNE memory usage and many other optimizer issues (#9032)
  FIX broken link in gallery and bad title rendering
  [MRG] DOC Replace \acute by prime (#9332)
  Fix typos (#9320)
  [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (#9206)
  DOC Residual sum vs. regression sum (#9314)
  [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (#9317)
  More informative error message for classification metrics given regression output (#9275)
  [MRG] COSMIT Remove unused parameters in private functions (#9310)
  [MRG+1] Ridgecv normalize (#9302)
  [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (#7388)
  Add data_home parameter to fetch_kddcup99 (#9289)
  FIX makedirs(..., exists_ok) not available in Python 2 (#9284)
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'dfsg' into debian
* dfsg: (808 commits)
  Preparing 0.19b2
  [MRG+1] FIX out of bounds array access in SAGA (#9376)
  FIX make test_importances pass on 32 bit linux
  Release 0.19b1
  DOC remove 'in dev' header in whats_new.rst
  DOC typos in whats_news.rst [ci skip]
  [MRG] DOC cleaning up what's new for 0.19 (#9252)
  FIX t-SNE memory usage and many other optimizer issues (#9032)
  FIX broken link in gallery and bad title rendering
  [MRG] DOC Replace \acute by prime (#9332)
  Fix typos (#9320)
  [MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring params and their order (#9206)
  DOC Residual sum vs. regression sum (#9314)
  [MRG] [HOTFIX] Fix capitalization in test and hence fix failing travis at master (#9317)
  More informative error message for classification metrics given regression output (#9275)
  [MRG] COSMIT Remove unused parameters in private functions (#9310)
  [MRG+1] Ridgecv normalize (#9302)
  [MRG + 2] ENH Allow `cross_val_score`, `GridSearchCV` et al. to evaluate on multiple metrics (#7388)
  Add data_home parameter to fetch_kddcup99 (#9289)
  FIX makedirs(..., exists_ok) not available in Python 2 (#9284)
  ...

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG + 1 (rv) + 1 (alex) + 1] Add a check to test the docstring param…
…s and their order (#9206)

* add automatic test of docstrings for function / method signatures using numpydoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment