Skip to content

[MRG] MNT make private the modules in sklearn.mixture #15166

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 6 commits into from
Oct 16, 2019

Conversation

NicolasHug
Copy link
Member

Towards #9250

@NicolasHug NicolasHug changed the title [WIP] MNT make private the modules in sklearn.mixture [MRG] MNT make private the modules in sklearn.mixture Oct 9, 2019
@NicolasHug
Copy link
Member Author

ping @adrinjalali @glemaitre @thomasjpfan for a hopefully easy review :)

@adrinjalali adrinjalali self-assigned this Oct 16, 2019
@@ -28,6 +28,10 @@
('sklearn.cluster.mean_shift_', 'MeanShift'),
('sklearn.cluster.optics_', 'OPTICS'),
('sklearn.cluster.spectral', 'SpectralClustering'),

('sklearn.mixture.base', 'BaseMixture'),
Copy link
Member

Choose a reason for hiding this comment

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

This was not in the __init__.py, why are we having it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we still support importing from sklearn.mixture.base for the next 2 versions

Copy link

Choose a reason for hiding this comment

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

does this mean that we cannot use them after the next 2 versions? it is because the security issue or something?

Copy link
Member

Choose a reason for hiding this comment

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

it means you can still import them from sklearn.mixture._base, but they're not a part of the public API and may change w/o a deprecation cycle. So your code may break between sklearn releases.

Copy link

Choose a reason for hiding this comment

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

Thank you very much!

@@ -24,6 +24,12 @@
('_mean_shift', 'sklearn.cluster.mean_shift_', 'sklearn.cluster'),
('_optics', 'sklearn.cluster.optics_', 'sklearn.cluster'),
('_spectral', 'sklearn.cluster.spectral', 'sklearn.cluster'),

('_base', 'sklearn.mixture.base', 'sklearn.mixture'),
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

sklearn/mixture/base.py was renamed into sklearn/mixture/_base.py, so yes.

If a user tries to from sklearn.mixture.base import something it will get a warning saying that it should instead import stuff from sklearn.mixture, and if that doesn't work that means that what they were importing is now private and they should not use it.

@adrinjalali adrinjalali merged commit 6ac9dc4 into scikit-learn:master Oct 16, 2019
@xihajun
Copy link

xihajun commented Jul 10, 2020 via email

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.

4 participants