MNT Makes modules in tree and ensemble private #14964

adrinjalali · 2019-09-12T10:19:56Z

Addresses part of #12927 and part of #9250

This PR renames file.py into _file.py in the sklearn.{tree, ensemble} modules.

In particular, please note that:

I rename tree.py to _tree_base.py since changing the _tree.{pyx,pxd}'s names would result in a much larger diff, same for gradient_boosting.py -> _gb.py

Ping @NicolasHug @thomasjpfan @rth @glemaitre.

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

NicolasHug

Did you make sure no examples import from ensemble.XXX or tree.XXX ?

As a nit I feel like tree_base.py isn't a great name because it actually contains all of the concrete tree classes (DecisionTreeClassifier, etc).
Maybe we can just rename the .pyx file instead (that's gonna be a bit annoying for you because you'll need a make clean and make in again. Also since tree.pyx isn't formally private it would need to be tested too).

LGTM anyway, thanks.

NicolasHug · 2019-09-12T12:52:57Z

sklearn/tests/test_import_deprecations.py

@@ -11,6 +11,30 @@
 # appropriate error message.

 @pytest.mark.parametrize('deprecated_path, importee', (
+    ('sklearn.tree.tree', 'DecisionTreeClassifier'),
+    ('sklearn.tree.tree', 'DecisionTreeRegressor'),


Side note, since we import * you only need to import one thing from the module in this test

This needs to be addressed. I do not think we need all these tests.

sklearn/ensemble/__init__.py

glemaitre · 2019-09-13T09:50:41Z

We might break some people code if they try to import the base class. The following will fail:

from sklearn.ensemble.bagging import BaseBagging

while I would argue that importing the _*** functions is not really our problem since they are private, the base class might still be used by people. We should probably add all base class in the __all__.

NicolasHug · 2019-09-13T11:12:11Z

good point, I didn't think of that.

Should we simply remove __all__ in _bagging.py?

glemaitre · 2019-09-13T11:16:33Z

Should we simply remove all in _bagging.py?

Or add whatever public classes/functions in __all__.

NicolasHug · 2019-09-13T11:26:59Z

A small issue I have with that is that we might miss some, and having them in all suggests that they should be publicly exposed somehow, but we are still making them private.

glemaitre · 2019-09-13T11:33:56Z

Actually, both approaches work for me. Removing __all__ might be safer then.

adrinjalali · 2019-09-14T10:42:54Z

Did you make sure no examples import from ensemble.XXX or tree.XXX ?

I think there's none, I checked, but I plan to do another round of warning removal and cleanup on the examples anyway.

As a nit I feel like tree_base.py isn't a great name because it actually contains all of the concrete tree classes (DecisionTreeClassifier, etc).
Maybe we can just rename the .pyx file instead (that's gonna be a bit annoying for you because you'll need a make clean and make in again. Also since tree.pyx isn't formally private it would need to be tested too).

I didn't rename the _tree.pyx cause it's used in so many different places that it would be a bigger change, also for people who use it. I know it's private, but I thought it'd be nice to avoid renaming that file.

adrinjalali · 2019-09-14T11:37:08Z

Question, which of these do we want to expose as public?

$ git grep "^class " sklearn/ensemble/ | grep -v tests 
sklearn/ensemble/_gb_losses.py:class LossFunction(metaclass=ABCMeta):
sklearn/ensemble/_gb_losses.py:class RegressionLossFunction(LossFunction, metaclass=ABCMeta):
sklearn/ensemble/_gb_losses.py:class LeastSquaresError(RegressionLossFunction):
sklearn/ensemble/_gb_losses.py:class LeastAbsoluteError(RegressionLossFunction):
sklearn/ensemble/_gb_losses.py:class HuberLossFunction(RegressionLossFunction):
sklearn/ensemble/_gb_losses.py:class QuantileLossFunction(RegressionLossFunction):
sklearn/ensemble/_gb_losses.py:class ClassificationLossFunction(LossFunction, metaclass=ABCMeta):
sklearn/ensemble/_gb_losses.py:class BinomialDeviance(ClassificationLossFunction):
sklearn/ensemble/_gb_losses.py:class MultinomialDeviance(ClassificationLossFunction):
sklearn/ensemble/_gb_losses.py:class ExponentialLoss(ClassificationLossFunction):
sklearn/ensemble/_hist_gradient_boosting/binning.py:class _BinMapper(BaseEstimator, TransformerMixin):
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py:class BaseHistGradientBoosting(BaseEstimator, ABC):
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py:class HistGradientBoostingRegressor(BaseHistGradientBoosting, RegressorMixin):
sklearn/ensemble/_hist_gradient_boosting/gradient_boosting.py:class HistGradientBoostingClassifier(BaseHistGradientBoosting,
sklearn/ensemble/_hist_gradient_boosting/grower.py:class TreeNode:
sklearn/ensemble/_hist_gradient_boosting/grower.py:class TreeGrower:
sklearn/ensemble/_hist_gradient_boosting/loss.py:class BaseLoss(ABC):
sklearn/ensemble/_hist_gradient_boosting/loss.py:class LeastSquares(BaseLoss):
sklearn/ensemble/_hist_gradient_boosting/loss.py:class BinaryCrossEntropy(BaseLoss):
sklearn/ensemble/_hist_gradient_boosting/loss.py:class CategoricalCrossEntropy(BaseLoss):
sklearn/ensemble/_hist_gradient_boosting/predictor.py:class TreePredictor:
sklearn/ensemble/_hist_gradient_boosting/splitting.pyx:class SplitInfo:
sklearn/ensemble/bagging.py:class BaseBagging(BaseEnsemble, metaclass=ABCMeta):
sklearn/ensemble/bagging.py:class BaggingClassifier(BaseBagging, ClassifierMixin):
sklearn/ensemble/bagging.py:class BaggingRegressor(BaseBagging, RegressorMixin):
sklearn/ensemble/base.py:class BaseEnsemble(BaseEstimator, MetaEstimatorMixin, metaclass=ABCMeta):
sklearn/ensemble/forest.py:class BaseForest(BaseEnsemble, MultiOutputMixin, metaclass=ABCMeta):
sklearn/ensemble/forest.py:class ForestClassifier(BaseForest, ClassifierMixin, metaclass=ABCMeta):
sklearn/ensemble/forest.py:class ForestRegressor(BaseForest, RegressorMixin, metaclass=ABCMeta):
sklearn/ensemble/forest.py:class RandomForestClassifier(ForestClassifier):
sklearn/ensemble/forest.py:class RandomForestRegressor(ForestRegressor):
sklearn/ensemble/forest.py:class ExtraTreesClassifier(ForestClassifier):
sklearn/ensemble/forest.py:class ExtraTreesRegressor(ForestRegressor):
sklearn/ensemble/forest.py:class RandomTreesEmbedding(BaseForest):
sklearn/ensemble/gradient_boosting.py:class QuantileEstimator:
sklearn/ensemble/gradient_boosting.py:class MeanEstimator:
sklearn/ensemble/gradient_boosting.py:class LogOddsEstimator:
sklearn/ensemble/gradient_boosting.py:class ScaledLogOddsEstimator(LogOddsEstimator):
sklearn/ensemble/gradient_boosting.py:class PriorProbabilityEstimator:
sklearn/ensemble/gradient_boosting.py:class ZeroEstimator:
sklearn/ensemble/gradient_boosting.py:class LossFunction(metaclass=ABCMeta):
sklearn/ensemble/gradient_boosting.py:class RegressionLossFunction(LossFunction, metaclass=ABCMeta):
sklearn/ensemble/gradient_boosting.py:class LeastSquaresError(RegressionLossFunction):
sklearn/ensemble/gradient_boosting.py:class LeastAbsoluteError(RegressionLossFunction):
sklearn/ensemble/gradient_boosting.py:class HuberLossFunction(RegressionLossFunction):
sklearn/ensemble/gradient_boosting.py:class QuantileLossFunction(RegressionLossFunction):
sklearn/ensemble/gradient_boosting.py:class ClassificationLossFunction(LossFunction, metaclass=ABCMeta):
sklearn/ensemble/gradient_boosting.py:class BinomialDeviance(ClassificationLossFunction):
sklearn/ensemble/gradient_boosting.py:class MultinomialDeviance(ClassificationLossFunction):
sklearn/ensemble/gradient_boosting.py:class ExponentialLoss(ClassificationLossFunction):
sklearn/ensemble/gradient_boosting.py:class VerboseReporter(object):
sklearn/ensemble/gradient_boosting.py:class BaseGradientBoosting(BaseEnsemble, metaclass=ABCMeta):
sklearn/ensemble/gradient_boosting.py:class GradientBoostingClassifier(BaseGradientBoosting, ClassifierMixin):
sklearn/ensemble/gradient_boosting.py:class GradientBoostingRegressor(BaseGradientBoosting, RegressorMixin):
sklearn/ensemble/iforest.py:class IsolationForest(BaseBagging, OutlierMixin):
sklearn/ensemble/voting.py:class _BaseVoting(_BaseComposition, TransformerMixin):
sklearn/ensemble/voting.py:class VotingClassifier(_BaseVoting, ClassifierMixin):
sklearn/ensemble/voting.py:class VotingRegressor(_BaseVoting, RegressorMixin):
sklearn/ensemble/weight_boosting.py:class BaseWeightBoosting(BaseEnsemble, metaclass=ABCMeta):
sklearn/ensemble/weight_boosting.py:class AdaBoostClassifier(BaseWeightBoosting, ClassifierMixin):
sklearn/ensemble/weight_boosting.py:class AdaBoostRegressor(BaseWeightBoosting, RegressorMixin):

NicolasHug · 2019-09-15T12:42:43Z

Everything that is already in ensemble/__init__.py is public, the rest becomes private.

adrinjalali · 2019-10-04T13:19:24Z

I still don't know what to call the new tree file. I really rather not rename _tree.pyx and _tree.pxd files, since not only it's engangled with the rest of the codebase, but also users who were importing things from tree._tree will get really odd errors after these renames.

thomasjpfan · 2019-10-04T13:46:02Z

What do you think about renaming tree.py into _base.py?

NicolasHug

Needs a merge but this still LGTM

NicolasHug · 2019-10-09T15:56:56Z

sklearn/_build_utils/deprecated_modules.py

@@ -7,7 +7,16 @@
 # (new_module_name, deprecated_path, correct_import_path)
 _DEPRECATED_MODULES = {
    # TODO: Remove in 0.24
-    ('_mocking', 'sklearn.utils.mocking', 'sklearn.utils')
+    ('_mocking', 'sklearn.utils.mocking', 'sklearn.utils'),


nit: skip line

thomasjpfan · 2019-10-09T16:06:35Z

I see that you undid the change to partial_dependence at f3e50d9 What was the reason for this?

NicolasHug · 2019-10-09T16:32:33Z

Also I don't mind too much about _base.py but in the svm module this is called _classes.py. Might make more sense since these are not just the base classes.

adrinjalali · 2019-10-15T09:31:46Z

I see that you undid the change to partial_dependence at f3e50d9 What was the reason for this?

Because we were not exposing anything from inside that file at the sklearn.ensemble level. I thought that requires it's own careful consideration.

NicolasHug · 2019-10-15T13:31:16Z

also everything from ensemble/partial_dependence is already deprecated so I agree we don't need to touch it.

@thomasjpfan let's merge?

adrinjalali added 7 commits September 11, 2019 18:32

rename the tree.py and export.py and make the tests pass

df7f54d

add the almost empty files

0d47064

add deprecation tests

d5f64aa

move files and local tests pass

e30d7dc

fix another import

fda6f20

add new files, and add warning tests

b31153a

not touching ensemble/partial_dependence

f3e50d9

NicolasHug reviewed Sep 12, 2019

View reviewed changes

NicolasHug approved these changes Sep 12, 2019

View reviewed changes

NicolasHug reviewed Sep 12, 2019

View reviewed changes

sklearn/ensemble/__init__.py Show resolved Hide resolved

glemaitre self-requested a review September 13, 2019 09:31

glemaitre removed their request for review September 13, 2019 09:52

merge master

63e5c97

adrinjalali added 4 commits October 4, 2019 13:58

remove public files

9c86b6e

merge upstream/master

1b50bfe

fix tests

aea646e

fix tree

0825a94

adrinjalali added 3 commits October 6, 2019 11:19

merge upstream/master

23bbfe1

fix test

4bf2b43

rename tree to _base.py

87374b8

...

a1e359b

NicolasHug reviewed Oct 9, 2019

View reviewed changes

merge master

037e352

rename _base -> _classes

47e5422

remove redundant tests

ac447a5

thomasjpfan approved these changes Oct 16, 2019

View reviewed changes

thomasjpfan changed the title ~~underscore files in {tree, ensemble} folder~~ MNT Makes modules in tree and ensemble private Oct 16, 2019

thomasjpfan merged commit 437ca05 into scikit-learn:master Oct 16, 2019

adrinjalali deleted the api/public/tree branch October 16, 2019 17:27

jnothman mentioned this pull request Oct 24, 2019

Use of private API will break with scikit-learn 0.22 scikit-learn-contrib/imbalanced-learn#616

Closed

ChemEngDataSci mentioned this pull request Apr 17, 2020

add sklearn.ensemble._gb.GradientBoostingClassifier to the list shap/shap#1162

Merged

yuchaoran2011 mentioned this pull request Jan 3, 2021

Incompatible with latest version of sklearn parrt/random-forest-importances#44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Makes modules in tree and ensemble private #14964

MNT Makes modules in tree and ensemble private #14964

adrinjalali commented Sep 12, 2019

NicolasHug left a comment

NicolasHug Sep 12, 2019

thomasjpfan Oct 15, 2019

adrinjalali Oct 16, 2019

glemaitre commented Sep 13, 2019

NicolasHug commented Sep 13, 2019

glemaitre commented Sep 13, 2019

NicolasHug commented Sep 13, 2019

glemaitre commented Sep 13, 2019

adrinjalali commented Sep 14, 2019

adrinjalali commented Sep 14, 2019

NicolasHug commented Sep 15, 2019

adrinjalali commented Oct 4, 2019

thomasjpfan commented Oct 4, 2019

NicolasHug left a comment

NicolasHug Oct 9, 2019

thomasjpfan commented Oct 9, 2019

NicolasHug commented Oct 9, 2019

adrinjalali commented Oct 15, 2019

NicolasHug commented Oct 15, 2019

MNT Makes modules in tree and ensemble private #14964

MNT Makes modules in tree and ensemble private #14964

Conversation

adrinjalali commented Sep 12, 2019

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Sep 12, 2019

Choose a reason for hiding this comment

thomasjpfan Oct 15, 2019

Choose a reason for hiding this comment

adrinjalali Oct 16, 2019

Choose a reason for hiding this comment

glemaitre commented Sep 13, 2019

NicolasHug commented Sep 13, 2019

glemaitre commented Sep 13, 2019

NicolasHug commented Sep 13, 2019

glemaitre commented Sep 13, 2019

adrinjalali commented Sep 14, 2019

adrinjalali commented Sep 14, 2019

NicolasHug commented Sep 15, 2019

adrinjalali commented Oct 4, 2019

thomasjpfan commented Oct 4, 2019

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Oct 9, 2019

Choose a reason for hiding this comment

thomasjpfan commented Oct 9, 2019

NicolasHug commented Oct 9, 2019

adrinjalali commented Oct 15, 2019

NicolasHug commented Oct 15, 2019