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.

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

FEA Allow passing custom covariance estimators to LDA #14446

Merged
merged 62 commits into from Sep 17, 2020
Merged

FEA Allow passing custom covariance estimators to LDA #14446

merged 62 commits into from Sep 17, 2020

Conversation

hugorichard
Copy link
Contributor

@hugorichard hugorichard commented Jul 23, 2019 •
edited by adrinjalali

Fixes #14436
Plug-in any covariance estimator into Fisher discriminants
This PR allows the user to plug-in any sklearn covariance estimator into LDA and QDA. This option is only possible with the "lsqr" and "eigen" solver of LDA and for the "lsqr" solver of QDA (initially only an "svd" solver was available for QDA so I made this explicit and added the "lsqr").

@hugorichard hugorichard changed the title Solve #14436 - Modular discriminant analysis [MRG] Solve #14436 - Modular discriminant analysis Jul 25, 2019
@adrinjalali adrinjalali changed the title [MRG] Solve #14436 - Modular discriminant analysis [MRG] Modular discriminant analysis Aug 7, 2019
@adrinjalali
Copy link
Member

Overall looks pretty good to me.

@hugorichard
Copy link
Contributor Author

@adrinjalali Thanks for your comments. I made the requested changes.

adrinjalali
adrinjalali reviewed Aug 29, 2019
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

A few comments for now, till the next round :) Thanks @hugorichard

build_tools/circle/flake8_diff.sh Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
@hugorichard
Copy link
Contributor Author

Thanks @adrinjalali I have made the changes.

adrinjalali
adrinjalali reviewed Sep 27, 2019
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Looking better now, thanks for the corrections @hugorichard

sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/tests/test_discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/tests/test_discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/tests/test_discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/tests/test_discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/tests/test_discriminant_analysis.py Outdated Show resolved Hide resolved
@cmarmo
Copy link
Member

cmarmo commented Aug 17, 2020

Hi @hugorichard , thanks for your patience! Do you mind triggering a new build either with an empty commit or syncing with upstream? The rendering of the documentation is not available anymore. Thanks!

@hugorichard
Copy link
Contributor Author

hugorichard commented Aug 17, 2020 •
edited

@cmarmo Thanks for your answer ! I have done what was asked.

cmarmo
cmarmo reviewed Aug 17, 2020
View reviewed changes
Copy link
Member

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @hugorichard minor comments waiting for a core-dev review.

examples/classification/plot_lda.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
sklearn/discriminant_analysis.py Outdated Show resolved Hide resolved
@hugorichard
Copy link
Contributor Author

@cmarmo Requested changes have been made

@cmarmo
Copy link
Member

cmarmo commented Aug 23, 2020

Thanks @hugorichard! @NicolasHug might want to give a second review? Thanks!

@hugorichard
Copy link
Contributor Author

@NicolasHug @cmarmo Any news on this ?

@cmarmo
Copy link
Member

cmarmo commented Sep 16, 2020

@NicolasHug, this is the oldest PR already approved waiting for reviewer and you already gave an LGTM (apart nitpicks.. :) ): may I ask you if you could find some time to finalize it? I'm happy to approve myself but it doesn't count for merging. Thanks for your patience!

@NicolasHug
Copy link
Member

NicolasHug commented Sep 16, 2020 •
edited

Thanks for the gentle reminders @hugorichard and @cmarmo

@hugorichard also thanks again for your patience. I have committed a few minor changes in https://github.com/NicolasHug/scikit-learn/tree/my_modular_discriminant_analysis, which I cannot directly pushed here (looks like you have deactivated this option on this PR). I only added 1 test and made some cosmetic changes.

Would you mind pulling the changes from my branch? LGTM then! Thanks again!

EDIT: alternatively I opened a PR in your fork hugorichard#1

NicolasHug
NicolasHug reviewed Sep 16, 2020
View reviewed changes
doc/modules/lda_qda.rst Outdated Show resolved Hide resolved
@hugorichard
Copy link
Contributor Author

hugorichard commented Sep 17, 2020 •
edited

Hi @NicolasHug @cmarmo Thanks for your reviews.
I merged your branch and updated the documentation so that it is clear that OAS yields to better results if the assumptions of the LDA classifier hold.

@NicolasHug NicolasHug changed the title [MRG+1] Modular discriminant analysis FEA Allow passing custom covariance estimators to LDA Sep 17, 2020
@NicolasHug NicolasHug merged commit 370e545 into scikit-learn:master Sep 17, 2020
@NicolasHug
Copy link
Member

thanks a lot for your work @hugorichard !

@hugorichard hugorichard deleted the modular_discriminant_analysis branch September 17, 2020 15:23
@hugorichard
Copy link
Contributor Author

hugorichard commented Sep 17, 2020 •
edited

You are welcome ! Thanks for the reviews !

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@hugorichard @NicolasHug @jayzed82
FEA Allow passing custom covariance estimators to LDA (scikit-learn#1…
2b494c7 
…4446)

Co-authored-by: hugo richard <hugo.richard@inria.fr>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
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

@agramfort agramfort agramfort left review comments

@rth rth rth left review comments

@NicolasHug NicolasHug NicolasHug left review comments

@cmarmo cmarmo cmarmo left review comments

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@hugorichard @adrinjalali @NicolasHug @cmarmo @jnothman @agramfort @rth @amueller