MNT make files private for cluster module by NicolasHug · Pull Request #14948 · scikit-learn/scikit-learn

NicolasHug · 2019-09-10T21:03:35Z

Addresses part of #12927 and part of #9250

This PR renames file.py into _file.py in the sklearn.cluster module.

In particular, please note that:

I rename dbscan_.py into _dbscan.py. Same for optics_ and mean_shift_
I renamed k_means_.py into _k_means.py. Since _k_means.pyx already existed, I had to rename it too. I chose _k_means_fast.pyx but no strong opinion. Same for hierarchical.

Ping @adrinjalali @thomasjpfan @rth @glemaitre.

Again, lint issue can be ignored and are triggered by new files being created.

adrinjalali

Not a thorough review yet.

sklearn/cluster/tests/test_optics.py

sklearn/tests/test_common.py

adrinjalali · 2019-09-11T16:00:36Z

sklearn/tests/test_import_deprecations.py

+    ('sklearn.cluster.k_means_', 'KMeans'),
+    ('sklearn.cluster.mean_shift_', 'MeanShift'),
+    ('sklearn.cluster.optics_', 'OPTICS'),
+    ('sklearn.cluster.spectral', 'SpectralClustering'),


Haven't checked if this list is complete.

This seems to be leading to some confusion.

Can we simplify this check by only doing a module import:

import {deprecated_path}

So we do not need the importee?

That would only check that

import sklearn.neural_network.rbm

raises a warning, but not that we can still import the stuff we used to from rbm. I think it's important to still ensure that

Then we need a comment somewhere saying "We only need to import one thing from each file"

adrinjalali · 2019-09-12T09:57:48Z

tests failing, and I just realized an issue with not raising a deprecation warning in pytest sessions, in contrast to ignoring those specific files: now we can't actually check if our examples and other places in the code are still using the deprecated path, we won't be issuing the warning during a pytest session.

rth · 2019-09-12T10:04:48Z

and I just realized an issue with not raising a deprecation warning in pytest sessions, in contrast to ignoring those specific files: now we can't actually check if our examples and other places in the code are still using the deprecated path, we won't be issuing the warning during a pytest session.

Right but examples don't use pytest so you should see them? For everything else, if it's imported at top level you would have seen it at collection time only once, anyway? Feel free to use the other approach is you think it's really preferable.

adrinjalali

Tests seem to be certificate issue. LGTM :)

NicolasHug · 2019-09-12T10:47:40Z

(I still need to check the examples as you suggested, I'll do that soon)

glemaitre · 2019-09-13T11:35:57Z

If you could also check if there is any base classes

…uster_dep_underscore

NicolasHug · 2019-09-13T12:29:33Z

I've removed __all__ in the privte _file.py now

adrinjalali · 2019-09-14T10:35:17Z

sklearn/cluster/__init__.py


-from .spectral import spectral_clustering, SpectralClustering
-from .mean_shift_ import (mean_shift, MeanShift,
+from ._spectral import spectral_clustering, SpectralClustering


I think the question is, do we want to include BaseSpectral here.

Or generally, any base class. I think we haven't decided on that, have we?

glemaitre · 2019-09-15T12:05:53Z

I think that we should. They are part of the public API, otherwise, it should be named _BaseSpectral (as _BaseComposition)

…

On Sat, 14 Sep 2019 at 12:36, Adrin Jalali ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/cluster/__init__.py <#14948 (comment)> : > @@ -3,18 +3,18 @@ algorithms. """ -from .spectral import spectral_clustering, SpectralClustering -from .mean_shift_ import (mean_shift, MeanShift, +from ._spectral import spectral_clustering, SpectralClustering I think the question is, do we want to include BaseSpectral here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14948?email_source=notifications&email_token=ABY32P3MCRB6WBZDTTRMWXDQJS5BPA5CNFSM4IVMRQQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEXVYGA#pullrequestreview-288316440>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY32P232UQKWXHVVORDAP3QJS5BPANCNFSM4IVMRQQQ> .

-- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/

NicolasHug · 2019-09-15T12:32:26Z

I am strongly against making base classes public. It's pretty much equivalent to making all the helpers public.

They are often very tightly coupled to the child classes, and having them public would make any refactoring or bug fix near impossible.

I'm pretty sure we've been consistently breaking backward compatibility of base classes.

Sort of related: that's another level of API contract, but any method that is not fit, transform, or something explicitly documented should be implicitly private too.

NicolasHug · 2019-09-15T12:34:08Z

They are part of the public API, otherwise, it
should be named _BaseSpectral (as _BaseComposition)

We have tons of things that are public but should have been private in the first place, this is precisely the point of these PRs: clean the public API space.

glemaitre · 2019-09-16T11:56:53Z

I am fine making the base classes private as well. However, this not the case now and we should follow the deprecation path as well, is that correct?

…

On Sun, 15 Sep 2019 at 14:34, Nicolas Hug ***@***.***> wrote: They are part of the public API, otherwise, it should be named _BaseSpectral (as _BaseComposition) We have tons of things that are public but should have been private in the first place, this is precisely the point of these PRs: clean the public API space. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14948?email_source=notifications&email_token=ABY32P7ERZOM5GFIKL7Z7TDQJYTW7A5CNFSM4IVMRQQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6XP3YA#issuecomment-531561952>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY32P23YYWTVWLB2RWU5XTQJYTW7ANCNFSM4IVMRQQQ> .

-- Guillaume Lemaitre INRIA Saclay - Parietal team Center for Data Science Paris-Saclay https://glemaitre.github.io/

NicolasHug · 2019-09-16T13:21:24Z

we should follow the deprecation path as well, is that correct?

We are properly deprecating all the base classes. All the tools that aren't in cluster.__init__ are now private (with deprecation), and base classes aren't in init

…uster_dep_underscore

adrinjalali · 2019-09-18T08:46:10Z

merge conflict and tests are failing.

NicolasHug · 2019-09-18T14:11:59Z

I'm leaving as-is for now, I think we need to discuss the conflicts issues #12927 (comment) on Monday during the meeting

adrinjalali · 2019-09-23T14:44:38Z

I kinda didn't understand what the conclusion on this topic was during the meeting.

Do we continue and comment on PRs telling people how to handle merge conflicts?

NicolasHug · 2019-09-23T14:57:38Z

@thomasjpfan is thinking of some solution that could avoid the conflicts... waiting for him to submit the idea

NicolasHug · 2019-10-07T20:30:00Z

Just updated the PR.

@thomasjpfan wanna give it a look?

jnothman · 2019-10-08T04:48:46Z

Where do you do the generation here?

thomasjpfan · 2019-10-08T05:15:28Z

Where do you do the generation here?

@jnothman It is done in sklearn/_build_utils/deprecated_modules.py by adding to the _DEPRECATED_MODULES set.

NicolasHug · 2019-10-09T15:54:20Z

Are you OK with this @thomasjpfan ?

@adrinjalali wanna give it a second round since method has changed?

jnothman

Otherwise I think this lgtm but I've not been rigorous

jnothman · 2019-10-10T07:42:08Z

sklearn/cluster/_bicluster.py

@@ -20,10 +20,6 @@
 from ..utils.validation import assert_all_finite, check_array


-__all__ = ['SpectralCoclustering',


This will technically mean that people who previously imported * from this module will now get check_array in their locals... Not sure that matters

True. Can't we just keep this? What's the point of removing it?

What's the point of removing it?

It suggests that users might be allowed to import from this. And there's no reason to protect users that want to import * IMO.

But OK I'll put it back.

sklearn/tests/test_import_deprecations.py

thomasjpfan · 2019-10-10T17:12:23Z

There is a linting error.

NicolasHug · 2019-10-10T17:15:54Z

There is a linting error.

It's going to happen in every deprecation PR. It's because new files are being created. I don't think we should fix them here (increases the possibilities of a painful merge)

…uster_dep_underscore

…-learn; branch 'master' of github.com:scikit-learn/scikit-learn into cluster_dep_underscore

NicolasHug · 2019-10-14T13:44:25Z

Since @jnothman seems to be +1 as well I'll merge tomorrow unless we have additional comments

rth

Haven't checked very thoroughly but LGTM, aside from one minor comment

rth · 2019-10-14T13:48:31Z

sklearn/cluster/_bicluster.py

@@ -20,10 +20,6 @@
 from ..utils.validation import assert_all_finite, check_array


-__all__ = ['SpectralCoclustering',


True. Can't we just keep this? What's the point of removing it?

rth · 2019-10-14T13:49:37Z

sklearn/cluster/_k_means.py

 from ..utils.validation import FLOAT_DTYPES
 from ..exceptions import ConvergenceWarning
-from . import _k_means
+from . import _k_means_fast as _k_means


I also find the naming *_fast.pyx files more meaningful than with underscores as it was done before.

NicolasHug · 2019-10-14T16:28:22Z

Thanks for the review @rth!

Comments are addressed so I'll merge

jnothman · 2019-10-24T03:43:37Z

My colleague, @namoshizun, has had trouble upgrading his nightly build installation due to this PR.

import sklearn.cluster when when trying to perform from ._k_means import k_means.

He had an installation of the scikit-learn nightly build prior to this PR, then upgraded that environment with pip install -U --pre -f https://sklearn-nightly.scdn8.secure.raxcdn.com scikit-learn. For unknown reasons, the existing _k_means module (_k_means.cpython-36m-darwin.so) was not removed when the prev version uninstalled. The import of _k_means then referenced this .so rather than the new _k_means.py.

I'm not sure why the old .so was not removed by pip, and I have not been able to replicate this. But I thought others should be aware.

thomasjpfan · 2019-10-24T04:06:21Z

I tried install the stable release and then pip install -U ... to install the dev build and was not able to produce this either.

Does uninstalling and then reinstall the dev version resolve @namoshizun's issue?

adrinjalali · 2019-10-24T10:10:22Z

I don't know what pip install -U exactly does, but I have had the exact same issue with make in, since those files are not removed. But it seems very odd that pip wouldn't remove the old ones.

NicolasHug added 4 commits September 10, 2019 16:27

first files

ee48807

some more

dddbf87

some more

6a84df7

fix import issue

50a5710

NicolasHug changed the title ~~[NOMRG] underscore files for cluster folder~~ [MRG] underscore files for cluster folder Sep 11, 2019

adrinjalali reviewed Sep 11, 2019

View reviewed changes

more sensible imports

b8da25a

adrinjalali approved these changes Sep 12, 2019

View reviewed changes

updated import examples

09446a5

NicolasHug added 2 commits September 13, 2019 08:24

Merge branch 'master' of github.com:scikit-learn/scikit-learn into cl…

a4a7266

…uster_dep_underscore

removed __all__ in now private modules

aee1ba7

adrinjalali reviewed Sep 14, 2019

View reviewed changes

NicolasHug added 2 commits September 16, 2019 09:47

Merge branch 'master' of github.com:scikit-learn/scikit-learn into cl…

54ca450

…uster_dep_underscore

aljefnjleanfnaef

f789bcc

glemaitre self-requested a review September 23, 2019 14:24

removed files since they will be generated

affef57

NicolasHug added 3 commits October 7, 2019 14:58

Merge branch 'master' into cluster_dep_underscore

7ec33db

some fixes

eeaf463

reduce diff

52501ec

gitignore update

92bd4d6

added one entry per file note

1dee188

thomasjpfan approved these changes Oct 9, 2019

View reviewed changes

jnothman reviewed Oct 10, 2019

View reviewed changes

typo

ddee10d

NicolasHug added 2 commits October 13, 2019 14:24

Merge branch 'master' of github.com:scikit-learn/scikit-learn into cl…

a478959

…uster_dep_underscore

Merge branch 'cluster_dep_underscore' of github.com:NicolasHug/scikit…

6c26316

…-learn; branch 'master' of github.com:scikit-learn/scikit-learn into cluster_dep_underscore

rth approved these changes Oct 14, 2019

View reviewed changes

put back __all__ in bicluster.py

2b29717

NicolasHug changed the title ~~[MRG] underscore files for cluster folder~~ MNT make files private for cluster module Oct 14, 2019

NicolasHug merged commit 14295f9 into scikit-learn:master Oct 14, 2019

NicolasHug deleted the cluster_dep_underscore branch October 14, 2019 16:29

TomAugspurger mentioned this pull request Oct 16, 2019

Avoid deprecation warning for cluster module #15276

Merged

NicolasHug pushed a commit that referenced this pull request Oct 16, 2019

MNT Avoid cluster warning following #14948 (#15276)

1fd0c34

stsievert mentioned this pull request Mar 30, 2020

test CI dask/dask-ml#631

Merged

		@@ -20,10 +20,6 @@
		from ..utils.validation import assert_all_finite, check_array


		__all__ = ['SpectralCoclustering',

Uh oh!

Conversation

NicolasHug commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Sep 12, 2019

Uh oh!

rth commented Sep 12, 2019

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Sep 12, 2019

Uh oh!

glemaitre commented Sep 13, 2019

Uh oh!

NicolasHug commented Sep 13, 2019

Uh oh!

adrinjalali Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 15, 2019 via email

Uh oh!

NicolasHug commented Sep 15, 2019

Uh oh!

NicolasHug commented Sep 15, 2019

Uh oh!

glemaitre commented Sep 16, 2019 via email

Uh oh!

NicolasHug commented Sep 16, 2019

Uh oh!

adrinjalali commented Sep 18, 2019

Uh oh!

NicolasHug commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Sep 23, 2019

Uh oh!

NicolasHug commented Sep 23, 2019

Uh oh!

NicolasHug commented Oct 7, 2019

Uh oh!

jnothman commented Oct 8, 2019

Uh oh!

thomasjpfan commented Oct 8, 2019

Uh oh!

NicolasHug commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomasjpfan commented Oct 10, 2019

Uh oh!

NicolasHug commented Oct 10, 2019

Uh oh!

NicolasHug commented Oct 14, 2019

NicolasHug commented Sep 10, 2019 •

edited

Loading

adrinjalali Sep 14, 2019 •

edited

Loading

NicolasHug commented Sep 18, 2019 •

edited

Loading

NicolasHug commented Oct 9, 2019 •

edited

Loading