8000 [MRG] Deprecate fit params in qda and lda by trevorstephens · Pull Request #5294 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Deprecate fit params in qda and lda #5294

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
Merged
merged 1 commit into from
Sep 22, 2015
Merged

[MRG] Deprecate fit params in qda and lda #5294

merged 1 commit into from
Sep 22, 2015

Conversation

trevorstephens
Copy link
Contributor

@amueller ... I believe this fixes #4107 and supercedes #4112 as LinearDiscriminantAnalysis and QuadraticDiscriminantAnalysis have moved to discriminant_analysis.py and that PR appears to have gone stale.

@trevorstephens
Copy link
Contributor Author

Oh btw I bumped the deprecation for LDA out one version since it wasn't really removed as an option in its fit() method. Happy to reel that back in to a version earlier, but this way they both dissolve at the same time.

@agramfort
Copy link
Member

LGTM

@arjoly
Copy link
Member
arjoly commented Sep 22, 2015

Since those class are brand new, can't we remove those without deprecation warnings?

otherwise lgtm

@agramfort
Copy link
Member
agramfort commented Sep 22, 2015 via email

@ogrisel
Copy link
Member
ogrisel commented Sep 22, 2015

No those classes are not brand new: they are renaming with the previous names deprecated.

sklearn.qda.QDA is a deprecated backward compatibility alias to sklearn.discriminant_analysis.QuadraticDiscriminantAnalysis.

So we need to do the per-parameter deprecation. Otherwise we would have to subclass QuadraticDiscriminantAnalysis as QDA to keep the old API only their while not having the deprecated fit parameters at all in QuadraticDiscriminantAnalysis.

But that sounds overly complex to me.

@ogrisel
Copy link
Member
ogrisel commented Sep 22, 2015

@trevorstephens can you please document the API change in what's new in the "API changes summary" section? This is a good opportunity to document the deprecated names of those 2 classes (I forgot to do it when merging the renaming).

@trevorstephens
Copy link
Contributor Author

Sure @ogrisel , just updated whatsnew and squashed.

@amueller amueller modified the milestones: 0.18, 0.17 Sep 22, 2015
amueller added a commit that referenced this pull request Sep 22, 2015
@amueller
Merge pull request #5294 from trevorstephens/lda_qda_fit_params
2a5652a 
[MRG] Deprecate fit params in qda and lda
@amueller amueller merged commit 2a5652a into scikit-learn:master Sep 22, 2015
@trevorstephens trevorstephens deleted the lda_qda_fit_params branch September 22, 2015 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Move all data-independent fit-params to __init__.
5 participants
@trevorstephens @agramfort @arjoly @ogrisel @amueller
0