Skip to content

[MRG+1] Fix LogisticRegression see also should include LogisticRegressiononCV #10022

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

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

srajanpaliwal
Copy link
Contributor

Reference Issues/PRs
Fixes #9995

What does this implement/fix? Explain your changes.
Added missing cross-references to CV estimators.

  • Added reference to LogisticRegressionCV in LogisticRegression.
  • Added reference to OrthogonalMatchingPursuitCV in OrthogonalMatchingPursuit.
  • Added references for ElasticNetCV, MultiTaskElasticNet, MultiTaskLasso.
  • Cross-referenced RFE and RFECV in each others docstring.
  • Cross-referenced CalibratedClassifierCV in _CalibrationClassifier
  • Added reference to LassoLarsIC in docstring of LassoLars.
  • Added description to references in See also section of Ridge, RidgeClassifier.

Any other comments?
While correcting this issue. I noticed that a lot of "See also" sections are missing short description about the references( some have descriptions and some don't). we can add short comment/description everywhere for consistency.

…onCV(scikit-learn#9995)

  -  Added reference to LogisticRegressionCV in LogisticRegression.
  -  Added reference to OrthogonalMatchingPursuitCV in OrthogonalMatchingPursuit.
  -  Added references for ElasticNetCV, MultiTaskElasticNet, MultiTaskLasso.
  -  Cross-referenced RFE and RFECV in each others docstring.
  -  Cross-referenced CalibratedClassifierCV in _CalibrationClassifier
  -  Added reference to LassoLarsIC in docstring of LassoLars.
  -  Added description to references in See also section of Ridge, RidgeClassifier.
@lesteve
Copy link
Member

lesteve commented Oct 27, 2017

Thanks a lot for your PR! LGTM

@lesteve lesteve changed the title [MRG] Fix LogisticRegression see also should include LogisticRegressiononCV [MRG+1] Fix LogisticRegression see also should include LogisticRegressiononCV Oct 27, 2017
Copy link
Member

@TomDLT TomDLT 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 this work, it's much appreciated !

we can add short comment/description everywhere for consistency.

It could be great. Would you like to do it?

RidgeClassifier, RidgeCV, :class:`sklearn.kernel_ridge.KernelRidge`
RidgeClassifier : Ridge classifier
RidgeCV : Ridge regression with built-in cross validation
:class:`sklearn.kernel_ridge.KernelRidge` : Kernel ridge regression
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use such syntax here?
What about just kernel_ridge.KernelRidge?

Copy link
Member

Choose a reason for hiding this comment

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

Note it was already like that before and the link works properly (I checked at some point I think). I am not sure but maybe this is to ensure there is a proper link in the doc. Wild guess: maybe the link is found automatically if the name is in the same module, but maybe not if it's another subpackage.

@lesteve
Copy link
Member

lesteve commented Oct 27, 2017

we can add short comment/description everywhere for consistency.

It could be great. Would you like to do it?

Personally I am in favour for merging small focussed PRs like this one before too much stuff is added to the PR and ultimately they become a pain to review. Also ultimately I don't think adding description for each name has so much added value, in the html it's a link so you can click on it. If you are looking at docstring you are probably knowledgeable enough to find the source where the symbol is defined.

@TomDLT
Copy link
Member

TomDLT commented Oct 27, 2017

Fair enough, merging

@TomDLT TomDLT merged commit 3d1b786 into scikit-learn:master Oct 27, 2017
@TomDLT
Copy link
Member

TomDLT commented Oct 27, 2017

Thanks @srajanpaliwal !

donigian added a commit to donigian/scikit-learn that referenced this pull request Oct 28, 2017
…cs/donigian-update-contribution-guidelines

* 'master' of github.com:scikit-learn/scikit-learn: (23 commits)
  fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033)
  [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025)
  [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022)
  [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015)
  MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573)
  [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005)
  [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399)
  [MRG] FIX bug in nested set_params usage (scikit-learn#9999)
  [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798)
  [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968)
  DOC Fix typo (scikit-learn#9996)
  DOC Fix typo: x axis -> y axis (scikit-learn#9985)
  improve example plot_forest_iris.py (scikit-learn#9989)
  [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875)
  DOC update news
  DOC Fix three typos in manifold documentation (scikit-learn#9990)
  DOC add missing dot in docstring
  DOC Add what's new for 0.19.1 (scikit-learn#9983)
  Improve readability of outlier detection example. (scikit-learn#9973)
  DOC: Fixed typo (scikit-learn#9977)
  ...
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
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.

LogisticRegression see also should include LogisticRegressionCV
3 participants