Skip to content

[MRG+1] Ledoit-Wolf behavior explanation#9500

Merged
jnothman merged 11 commits intoscikit-learn:masterfrom
GKjohns:ledoit_wolf_explaination
Oct 8, 2017
Merged

[MRG+1] Ledoit-Wolf behavior explanation#9500
jnothman merged 11 commits intoscikit-learn:masterfrom
GKjohns:ledoit_wolf_explaination

Conversation

@GKjohns
Copy link
Copy Markdown
Contributor

@GKjohns GKjohns commented Aug 5, 2017

Reference Issue

Fixes Issue #6482: Explanation of unexpected but correct behavior of Ledoit-Wolf covariance estimate. Originally raised by @clamus.

What does this implement/fix? Explain your changes.

The fix adds a brief explanation of some unexpected (but correct) behavior in implementations of the Ledoit-Wolf covariance estimator. The changes are to docstrings and a section of the module documenation—no implementation changes.

The estimator determines the optimal amount of shrinkage towards some multiple identity matrix for a given dataset. If the empirical covariance matrix is already a multiple of the covariance matrix, the estimator will interpret the result as having a high degree of shrinkage. This behavior has no effect on the resulting matrix.

Any other comments?

Sorry for the typo in the branch name. Looking forward to working with you in the Fall Professor @amueller!

@amueller
Copy link
Copy Markdown
Member

amueller commented Aug 10, 2017

this seems ok to me, but I'm not an expert in this one. ping @ogrisel, maybe @agramfort ?

NelleV
NelleV reviewed Aug 11, 2017
View reviewed changes
doc/modules/covariance.rst Outdated
the case. When the population covariance is a multiple of the
identity, the LW shrinkage estimate becomes close or equal to 1.
This indicates that the optimal estimate of the covariance matrix in
the LW sense of the is multiple of the identity. Since the population
Copy link
Copy Markdown
Member

@NelleV NelleV Aug 11, 2017

Choose a reason for hiding this comment

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

There is a problem with this sentence. Either part of it is missing, or there is a "of the" that needs to be deleted.

jnothman
jnothman reviewed Aug 20, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I know very little about this. Is the issue so prevalent as to deserve such prominent and repeated alerts? We tend to keep things brief in docstrings. If it remains in the docstring it should be in Notes below.

Even in the user guide, I wonder if it should be a .. note::.

Thanks

@GKjohns
Copy link
Copy Markdown
Contributor Author

GKjohns commented Aug 22, 2017

@jnothman That makes much more sense. I'd imagine the behavior would only arise in a handful of cases, so there's probably no need for anything more than a note in the documentation

jnothman
jnothman reviewed Aug 22, 2017
View reviewed changes
doc/modules/covariance.rst Outdated
to data.

* See :ref:`sphx_glr_auto_examples_covariance_plot_lw_vs_oas.py` to visualize the
* See :ref:`sphx_glr_auto_examples_covariance_plot_Ledoit-Wolf_vs_oas.py` to visualize the
Copy link
Copy Markdown
Member

@jnothman jnothman Aug 22, 2017

Choose a reason for hiding this comment

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

You've broken these links...

jnothman
jnothman reviewed Aug 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

@jnothman jnothman changed the title [MRG] Ledoit-Wolf behavior explanation [MRG+1] Ledoit-Wolf behavior explanation Aug 22, 2017
@GKjohns
Copy link
Copy Markdown
Contributor Author

GKjohns commented Oct 6, 2017

Made the changes. Is there anything else I should do for [MRG+2]?

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 8, 2017

I think we can merge this with one review. Thanks

@jnothman jnothman merged commit a0dfa30 into scikit-learn:master Oct 8, 2017
@GKjohns GKjohns deleted the ledoit_wolf_explaination branch October 8, 2017 05:33
amueller pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 20, 2017
@GKjohns @amueller
[MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
dacf9e3 
* DOC add explaination of unexpected behavior to ledoit-wolf functions and class

* DOC add explaination of unexpected ledoit-wolf behavior to module documentation

* fix line that's longer than 80 chars, pep8 issue

* fix documentation changes to Ledoit_Wolf behavior explaination

* change bahavior explanation to a note in documentation

* remove unexpected behavior explanation from docstrings

* fix broken links in docs
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge tag '0.19.1' into releases
40884af 
* tag '0.19.1': (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge branch 'releases' into dfsg
0cad75b 
* releases: (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge branch 'dfsg' into debian
25061b8 
* dfsg: (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
@qinhanmin2014 qinhanmin2014 mentioned this pull request Oct 31, 2017
Closed
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@GKjohns @maskani-moh
[MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
b2b92b3 
* DOC add explaination of unexpected behavior to ledoit-wolf functions and class

* DOC add explaination of unexpected ledoit-wolf behavior to module documentation

* fix line that's longer than 80 chars, pep8 issue

* fix documentation changes to Ledoit_Wolf behavior explaination

* change bahavior explanation to a note in documentation

* remove unexpected behavior explanation from docstrings

* fix broken links in docs
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@GKjohns
[MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
6b5a6c2 
* DOC add explaination of unexpected behavior to ledoit-wolf functions and class

* DOC add explaination of unexpected ledoit-wolf behavior to module documentation

* fix line that's longer than 80 chars, pep8 issue

* fix documentation changes to Ledoit_Wolf behavior explaination

* change bahavior explanation to a note in documentation

* remove unexpected behavior explanation from docstrings

* fix broken links in docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman left review comments

+1 more reviewer

@NelleV NelleV NelleV left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GKjohns @amueller @jnothman @NelleV