Init parameter check of estimator_checks overly restrictive #17756

timokau · 2020-06-27T13:41:10Z

Describe the bug

I'm working on a custom estimator that needs an optimizer as one of its hyperparameters. I'm using the pattern of passing in an uninitialized optimizer and its parameters separately, i.e.

my_estimator = MyEstimator(optimizer=SGD, optimizer__lr=1e-5)

which will then initialize the optimizer as needed. I'm attempting to verify this with the check_parameters_default_constructible test, but it fails due to this assertion:

scikit-learn/sklearn/utils/estimator_checks.py

Line 2624 in cf4fa5c

assert init_param.default in [np.float64, np.int64]

which restricts type arguments to numpy floats or numpy ints. I don't understand why this restriction is necessary. With some digging through the history, I found that the general type checks were added in 90d5ef1 to verify that all parameters are of immutable types. This was then extended by the restriction on type parameters in ab2f539, as a drive-by change without explanation. Since types are immutable, I don't understand why this was necessary.

I'm not sure if this is a bug or I am missing something. CC @amueller who added that check originally.

Steps/Code to Reproduce

Define an estimator such as

from keras.optimizers import SGD
from sklearn.base import BaseEstimator
from sklearn.utils.estimator_checks import check_parameters_default_constructible

class MyEstimator(BaseEstimator):
	def __init__(self, optimizer=SGD, **kwargs):
		self.optimizer=optimizer

check_parameters_default_constructible("default_constructible", MyEstimator)

Expected Results

The test passes since the estimator is default constructible and all arguments are of immutable type (the optimizer itself is of course not immutable, but the type is).

Actual Results

$ python3 example.py
Using TensorFlow backend.
WARNING:root:Limited tf.compat.v2.summary API due to missing TensorBoard installation.
Traceback (most recent call last):
  File "tmp.py", line 9, in <module>
    check_parameters_default_constructible("default_constructible", MyEstimator)
  File "/nix/store/isa1ilgb10xpkm6hjxaaw9m7g1xiiqp1-python3-3.7.7-env/lib/python3.7/site-packages/sklearn/utils/estimator_checks.py", line 2533, in check_parameters_default_constructible
    assert init_param.default in [np.float64, np.int64]
AssertionError

Versions

System:
    python: 3.7.7 (default, Mar 10 2020, 06:34:06)  [GCC 9.3.0]
executable: /nix/store/isa1ilgb10xpkm6hjxaaw9m7g1xiiqp1-python3-3.7.7-env/bin/python3.7
   machine: Linux-5.4.46-x86_64-with

Python dependencies:
       pip: None
setuptools: 45.2.0.post20200508
   sklearn: 0.22.2.post1
     numpy: 1.18.3
     scipy: 1.4.1
    Cython: None
    pandas: 1.0.3
matplotlib: 3.2.1
    joblib: 0.14.1

Built with OpenMP: True

(the relevant code hasn't changed in current sklearn master though).

The text was updated successfully, but these errors were encountered:

glemaitre · 2020-06-29T07:36:48Z

The way to go would be:

from keras.optimizers import SGD
from sklearn.base import BaseEstimator
from sklearn.utils.estimator_checks import check_parameters_default_constructible

class MyEstimator(BaseEstimator):
    def __init__(self, optimizer=None, **kwargs):
        self.optimizer=optimizer

    def fit(self, X, y):
        if self.optimizer is None:
            self.optimizer_ = SGD()
        else:
            self.optimizer_ = clone(self.optimizer)  # or make a deep copy to not change the passed optimizer

check_parameters_default_constructible("default_constructible", MyEstimator)

timokau · 2020-07-01T13:51:35Z

That would be a workaround for the test, but why would that be better? It is less readable (implicit SGD default specified with None). The question is why specifying SGD is prohibited in the first place.

rth · 2020-07-01T14:57:48Z

A agree that that validation list seems a bit arbitrary. Currently it would pass with int, but fail with np.int32, pass with np.float64 but fail with np.float32.

Maybe we could check for np.core.numerictypes.genericTypeRank (excluding object and complex) and also for objects that verify isinstance(obj, type) ? We already accept functions so why not class constructors as long as they are not constructed in __init__.

timokau · 2020-07-01T15:17:29Z

I have already made some changes locally to produce better error messages and make the test failures a bit more actionable & explanatory. Should I add those relaxations to it and make a PR?

rth · 2020-07-01T21:10:52Z

Let's wait for one more opinion to confirm (@NicolasHug ?) (maybe I'm forgetting some use case) and then a PR would be very welcome.

thomasjpfan · 2020-07-05T19:54:57Z

This part of the check feels very restrictive. I believe the point is to make sure that the estimator can be constructed by completely based on the __init__ parameters. In the case of optimizer=SGD, the check only needs to know if SGD() is works.

This feel like this can get more complicated if the parameters start to depend on each other. For example in the original post:

my_estimator = MyEstimator(optimizer=MySGD, optimizer__lr=1e-5)

What if MySGD(lr=1e-5) is valid, but MySGD() is not.

timokau · 2020-07-07T17:34:43Z

I believe the point is to make sure that the estimator can be constructed by completely based on the __init__ parameters.

Its also to make sure all parameters are immutable, which is why it makes sense to forbid arbitrary objects such an initialized SGD.

What if MySGD(lr=1e-5) is valid, but MySGD() is not.

Why does that matter? As long as lr is provided in the defaults, its still default constructible.

thomasjpfan · 2020-07-12T19:29:14Z

In this context of optimizers, there are optimizers that are require some parameters during __init__. For example, CyclicLR, which requires base_lr, and max_lr.

MyEstimator(optimizer= CyclicLR, optimizer__base_lr=1e-5, optimizer__ max_lr=1e-4)

MyEstimator can construct CyclicLR because it was given optimizer__*.

timokau · 2020-07-13T15:12:27Z

MyEstimator can construct CyclicLR because it was given optimizer__*.

But that means MyEstimator is default constructible, which is all the test is supposed to check right? Specifying optimizer__* in the defaults has other issues (need a way to invalidate them when optimizer is set), but that's a different problem.

thomasjpfan · 2020-07-15T17:04:53Z

But that means MyEstimator is default constructible, which is all the test is supposed to check right?

How would a check know that CyclicLR is default constructible? Currently the check does not do any special passing of optimizer__base_lr into CyclicLR.

timokau · 2020-07-15T17:47:19Z

But that means MyEstimator is default constructible, which is all the test is supposed to check right?

How would a check know that CyclicLR is default constructible? Currently the check does not do any special passing of optimizer__base_lr into CyclicLR.

That is a valid point, but I would argue that this goes beyond default constructibility. The Estimator is default constructible. It may break at fit time when it actually initializes its internal optimizer, but there are many other factors which may cause it to break at runtime. I think that is out of scope for the check.

thomasjpfan · 2020-07-15T18:49:40Z

thomasjpfan commented

Jul 15, 2020

Okay lets move forward with the suggestion by @rth #17756 (comment) to be more accepting of defaults.

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

timokau · 2020-07-16T17:30:43Z

The PR is now ready for review: #17936

timokau added the Bug: triage label Jun 27, 2020

This was referenced Jun 27, 2020

Require uninitialized optimizers for our learners kiudee/cs-ranking#119

Merged

[WIP] Make our estimators compatible with scikit-learn kiudee/cs-ranking#116

Draft

timokau mentioned this issue Jul 1, 2020

Misc fixes for default-constructibility of our learners kiudee/cs-ranking#144

Merged

7 tasks

timokau added a commit to timokau/scikit-learn that referenced this issue Jul 16, 2020

Relax init parameter type checks

e394d93

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

timokau mentioned this issue Jul 16, 2020

[MRG] Relax init parameter checks and add explanatory error messages #17936

Merged

timokau added a commit to timokau/scikit-learn that referenced this issue Jul 16, 2020

Relax init parameter type checks

3ea3278

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

timokau added a commit to timokau/scikit-learn that referenced this issue Jul 16, 2020

Relax init parameter type checks

63032a3

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

timokau added a commit to timokau/scikit-learn that referenced this issue Jul 16, 2020

Relax init parameter type checks

8ae44db

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

timokau added a commit to timokau/scikit-learn that referenced this issue Jul 16, 2020

Relax init parameter type checks

667efd9

We now allow any "type" (uninitialized classes) and all numeric numpy types. See scikit-learn#17756 for a discussion.

rth closed this as completed in #17936 Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init parameter check of estimator_checks overly restrictive #17756

Init parameter check of estimator_checks overly restrictive #17756

Init parameter check of estimator_checks overly restrictive #17756

Init parameter check of estimator_checks overly restrictive #17756

Comments

Describe the bug

Steps/Code to Reproduce

Expected Results

Actual Results

Versions