ENH Do not pass random_state to estimator in IterativeImputer by david-cortes · Pull Request #15636 · scikit-learn/scikit-learn

david-cortes · 2019-11-15T19:33:38Z

Trying to fix issue: #15611

Lots of algorithms that rely on randomization offer an argument random_state through which it's possible to pass them RNG seeds that would ensure reproducible results. The IterativeImputer class will take some regressor/classifier and set their attribute random_state to a NumPy MT19937 object class if the object has such attribute, in such a way that this object is modified implicitly once the regressor/classifier to which it was assigned produces some random number.

This is fine for SciKit-Learn’s own classes, but other libraries which provide different regressors/classifiers which are SciKit-Learn-compatible might not be able to use such RandomState objects (e.g. if they generate random numbers in C++ with some method other than MT19937).

This PR addresses the issue by setting the random_state of the estimator to ~~a random integer, which is drawn from the imputer's RandomState object~~ the same value that was passed to the imputer, but does so only if it's not already set in the estimator. This way:

It doesn't override the random_state which is passed to the constructor of the estimator used for imputation (if it was passed).
Allows to use a wider array of SciKit-Learn-compatible objects from external libraries as estimators for IterativeImputer. Example:

import numpy as np
from lightgbm import LGBMRegressor
from sklearn.experimental import enable_iterative_imputer
from sklearn.impute import IterativeImputer

np.random.seed(123)
X = np.random.normal(size = (100, 5))
X[np.random.randint(100, size = 10),
  np.random.randint(5,   size = 10)] = np.nan

imputer = IterativeImputer(LGBMRegressor(), random_state = 456)
X_full = imputer.fit_transform(X)

rth

Thanks! It would be good to have a unit test for this behaviour.

One concern is that this is not fully backward compatible (because the same random state will now produce different output) -- let's wait for more feedback to see if this is fine. I think it might be if we consider this a "bug fix".

rth · 2019-11-15T19:40:23Z


        if hasattr(self._estimator, 'random_state'):
-            self._estimator.random_state = self.random_state_
+            if self._estimator.random_state is None:


Shouldn't this be

Suggested change

if self._estimator.random_state is None:

if self._estimator.random_state is None and self.random_state_ is not None:

?

I don't think so, because self.random_state_ (with underscore) is returned from function check_random_state through self.random_state (no underscore), so it cannot be missing (L541).

rth · 2019-11-15T19:43:06Z

        if hasattr(self._estimator, 'random_state'):
-            self._estimator.random_state = self.random_state_
+            if self._estimator.random_state is None:
+                self._estimator.random_state = self.random_state_.randint(np.iinfo(np.int32).max)


Linting is failing because the line is too long

also this should probably be using set_params. we might special-case the way we handle random_state in the future as discussed in #14042

jnothman · 2019-11-17T11:39:54Z

I've not checked this out yet, but backwards incompatibility is not an issue as IterativeImputer is experimental

david-cortes · 2019-11-20T18:42:43Z

Changed to set_params now, and broke the long line to confirm to PEP8 limit.

adrinjalali · 2019-12-12T21:32:37Z

shouldn't iterative imputer pass the given random state to the underlying estimator in general?

We shouldn't try to convert it to an integer, we just pass it along (the original random_state). If the underlying model doesn't support an rng object, the user shouldn't pass that to IterativeImputer.

I think I'm missing something here though.

NicolasHug · 2019-12-12T21:38:14Z

If the underlying model doesn't support an rng object, the user shouldn't pass that to IterativeImputer.

That means that by default (i.e. with random_state=None), the IterativeImputer isn't compatible with third party estimators that only support ints.

Is it really part of our convention that estimators must support RandomState and None?

david-cortes · 2019-12-12T21:39:14Z

shouldn't iterative imputer pass the given random state to the underlying estimator in general?

We shouldn't try to convert it to an integer, we just pass it along (the original random_state). If the underlying model doesn't support an rng object, the user shouldn't pass that to IterativeImputer.

I think I'm missing something here though.

That's precisely the point of the PR: Estimators from outside scikit-learn which are scikit-learn-compatible oftentimes don't work with NumPy's RandomState object, which reduces the array of choices a lot, for no good reason. IterativeImputer still makes random choices on its own, so it needs to have a RandomState object created inside, even if you pass it an integer for random_state.

adrinjalali · 2019-12-12T23:37:50Z

If the user passes None as the random state to IterativeImputer, we don't have to set the random state of the underlying estimator at all. It will stay whatever the default of that estimator is. If that estimator doesn't support None then its default should be an int, and that will be the one used.

david-cortes · 2019-12-13T09:35:55Z

Updated. Now it will only set the random_state of the Estimator to an integer (not to a RandomState) if:

The estimator has attribute random_state.
The estimator's random_state is None.
IterativeImputer was passed a random_state other than None.

EDIT: fixed mixed use of spaces and tabs now.

EDIT2: corrected yet again for linting.

NicolasHug · 2019-12-13T10:04:34Z

If the user passes None as the random state to IterativeImputer, we don't have to set the random state of the underlying estimator at all.

That's not the semantic we've been using so far. None doesn't mean "no random state", it mean "use the numpy singleton".

adrinjalali · 2019-12-14T19:22:47Z

could you please avoid force pushing to the branch? makes it hard to keep track of the changes

adrinjalali · 2019-12-14T19:24:04Z

This also needs tests

david-cortes · 2019-12-16T15:16:01Z

Added tests for the cases in which it should and shouldn't set a seed in the estimator, verifying that the seed is an integer.

adrinjalali · 2020-01-02T15:14:21Z

+            random_state = self.random_state_.randint(
+                np.iinfo(np.int32).max)
+            self._estimator.set_params(random_state=random_state)


This should simply be:

self._estimator.set_params(random_state=self.random_state)

If the given estimator doesn't support int or rng or None, then user shouldn't pass that as the random state.

david-cortes · 2020-01-04T20:12:13Z

Modified to pass to the estimator the same random_state that was passed to the imputer instead of drawing a random integer from the internal RandomState.

adrinjalali

this also needs a whats_new entry, otherwise looks good to me.

adrinjalali · 2020-01-06T09:38:29Z

+
+        def predict(self, X):
+            return np.zeros(X.shape[0])
+    X = np.ones((10, 3), dtype=float)


a new line just for clarity

david-cortes · 2020-01-06T20:37:25Z

Added the extra line and comment on the changes in What's New.

jnothman · 2020-01-07T12:41:56Z

+:mod:`sklearn.impute`
+...............................
+
+- |Enhancement| :class:`impute.IterativeImputer` Will not share the internal


I think this is too much detail and is unclear in telling the user what they need to know. Perhaps "IterativeImputer now provides a random seed to its regressor instances, rather than a RandomState object." or "Fixes a bug where ..."?

jnothman · 2020-01-09T07:57:46Z

+  `RandomState` object with the estimator, but instead will set its attribute
+  `random_state` if it was passed to the imputer and not to the estimator,
+  allowing to use estimators which do not support `RandomState` objects.
+  :pr: `15636` by :user:`David Cortes` <david-cortes>.


Suggested change

:pr: `15636` by :user:`David Cortes` <david-cortes>.

:pr: `15636` by :user:`David Cortes <david-cortes>`.

Good catch! I see some other entries in there are also not entered this way and don’t render correctly (PR’s 15503 and 15918) - may I attempt to correct them too?

sure in a new PR, thanks

jnothman · 2020-01-09T07:58:48Z

+            and self._estimator.random_state is None
+            and self.random_state is not None
+        ):
+            self._estimator.set_params(random_state=self.random_state)


this is not necessarily an integer. should we instead be generating a random seed and passing that?

It was like that before, and I asked to change it. I don't think we should get a seed and pass around. If the estimator doesn't support rng, the user shouldn't pass an rng. I don't see why
we should workaround the API limitations of third party libraries.

I am happy with the current approach, where the int should be used if self._estimator only supports random_state ints.

Sorry for bringing this point late. I am wondering if IterativeImputer should be setting the random_state in self._estimator at all. In the example the user can do:

imputer = IterativeImputer(LGBMRegressor(random_state=42), random_state = 456)

In this version, IterativeImputer would not set self._estimator.random_state.

What do you think @david-cortes ?

I guess it's also a very valid approach (not setting the estimator's random_state), although it might cause some confusion when one has a seed/state set while the other doesn't. However in that snippet (setting the random_state in the estimator before passing it to the imputer), with the changes in this PR, it wouldn't set the estimator's random_state either, because it isn't None.

When looking at metaestimators such as RegressorChain of ClassifierChain, it has a random_state parameter, but does not set the random_state of the base_estimator. I think not setting random_state would be more consistent with the rest of the library.

jnothman · 2020-01-09T08:02:09Z

+
+        def fit(self, X, y):
+            if self.rs_expect == "RandomState":
+                assert isinstance(self.random_state, np.random.RandomState)


can't you check the identity of the RandomState?

It's not possible unfortunately as they contain information about memory address. So something like this would return False:

import numpy as np np.random.RandomState(100) == np.random.RandomState(100) >>> False

We can try to check for the state explicitly:

import numpy as np from numpy.testing import assert_array_equal rng1 = np.random.RandomState(100) rng2 = np.random.RandomState(100) rng1_state = rng1.get_state() rng2_state = rng2.get_state() assert rng1_state[0] == rng2_state[0] assert_array_equal(rng1_state[1], rng2_state[1]) assert_array_equal(rng1_state[2:], rng2_state[2:])

jnothman · 2020-01-09T08:04:48Z

+     (None, "string", "string"), (408, "string", "string"),
+     (np.random.RandomState(seed=409), "string", "string")]
+)
+def test_iterative_imputer_set_estimator_random_state(


I am finding it hard to read this test. I think naming and semantics of rs_expect could be improved at least.

david-cortes · 2020-01-10T12:32:34Z

Renamed the parameters in the test and added more line breaks for readability now. Also shortened the description in what's new.

NicolasHug

Do we still need self.random_state_ now?

NicolasHug · 2020-01-11T12:31:30Z

+)
+def test_iterative_imputer_set_estimator_random_state(
+    rs_imputer, rs_estimator, expect_in_fit
+):


Please add a short comment of what this test does

NicolasHug · 2020-01-11T12:35:30Z

+- |Enhancement| :class:`impute.IterativeImputer` Will set the estimator's
+  `random_state` to the same `random_state` that was passed to the imputer.


in contrast to what? what does this enable?

david-cortes · 2020-01-11T19:56:55Z

Do we still need self.random_state_ now?

Yes, it's still needed because the imputer itself also makes random choices such as the column order, so it needs an internal RandomState from which to generate random numbers.

david-cortes · 2020-01-11T20:24:22Z

Added a better description to the test, the what's new, and also to the documentation just in case. The merge conflict now is because of this other PR: #16091 which fixed spacing in the previous entry in what's new.

Not sure how to fix it in this pull request (other than force-pushing). Tips appreciated.

cmarmo · 2020-07-10T13:27:16Z

Hello @david-cortes , if you are still interested in working on this, do you mind fixing conflicts? This will bring more attention from the reviewer side. Thanks for your work!

… whitespace to its test

david-cortes · 2020-08-16T18:39:46Z

Solved merge conflicts.

thomasjpfan · 2020-08-16T19:21:08Z

+:mod:`sklearn.impute`
+...............................
+
+- |Enhancement| :class:`impute.IterativeImputer` Will set the estimator's


Suggested change

- |Enhancement| :class:`impute.IterativeImputer` Will set the estimator's

- |Enhancement| :class:`impute.IterativeImputer` will set the estimator's

thomasjpfan · 2020-08-16T19:23:50Z

+        ``sample_posterior`` is True. If the estimator has a `random_state`,
+        but it's set to None, will set it to this value too. Use an integer


Suggested change

``sample_posterior`` is True. If the estimator has a `random_state`,

but it's set to None, will set it to this value too. Use an integer

``sample_posterior`` is True. If the estimator has a `random_state`,

and it is set to None, then it's `random_state` will set it to this value

too. Use an integer for determinism.

thomasjpfan · 2020-08-16T19:26:38Z



+@pytest.mark.parametrize(
+    "rs_imputer, rs_estimator, expect_in_fit",


Suggested change

"rs_imputer, rs_estimator, expect_in_fit",

"rs_imputer, rs_estimator, rs_expect",

And the rest of the test funcion.

david-cortes · 2020-08-16T20:07:27Z

Incorporated suggestions.

david-cortes · 2020-08-17T18:40:00Z

Changed the logic of the PR to avoid setting the estimator's random_state at all in order to be more consistent with the other similar classes as suggested by @thomasjpfan . Since now it will not do anything with it I removed the test as it doesn't have anything to test now.

thomasjpfan

Thank you for the updates @david-cortes !

@adrinjalali What do you think of the recent changes here?

adrinjalali

Strangely enough it seems this change doesn't require a change in the random_state's description. Seems like a fix to me.

Still feels odd not to have a test for this change. We could test that the IterativeImputer does not set the random_state of a child estimator, otherwise LGTM.

I haven't read the whole conversation, is @NicolasHug happy with the change?

NicolasHug

thanks for your patience @david-cortes

NicolasHug · 2020-08-21T22:44:44Z

  with `add_indicator=True`.
  :pr:`17612` by :user:`Srimukh Sripada <d3b0unce>`

+- |Enhancement| :class:`impute.IterativeImputer` will not attempt to set the


As noted by Adrin this should probably be a |Fix| entry since the behaviour might change a little

david-cortes · 2020-08-22T08:20:24Z

Changed the what's new entry to "Fix" and added a test asserting that the imputer doesn't set the random_state of the estimator.

thomasjpfan · 2020-08-22T17:45:35Z

+@pytest.mark.parametrize(
+    "rs_imputer, rs_estimator",
+    [
+        (None, None),
+        (1, None),
+        (np.random.RandomState(seed=1), None),
+        (None, 1),
+        (1, 1),
+        (np.random.RandomState(seed=1), 1),
+        (None, np.random.RandomState(seed=1)),
+        (1, np.random.RandomState(seed=1)),
+        (np.random.RandomState(seed=1), np.random.RandomState(seed=1))
+    ]
+)
+def test_iterative_imputer_dont_set_random_state(rs_imputer, rs_estimator):


We can parameterize as follows: (May require adjusting the whitespace to pass the linter)

@pytest.mark.parametrize("rs_imputer", [None, 1, np.random.RandomState(seed=1)]) @pytest.mark.parametrize("rs_estimator", [None, 1, np.random.RandomState(seed=1)]) def test_iterative_imputer_dont_set_random_state(rs_imputer, rs_estimator):

david-cortes · 2020-08-22T18:08:06Z

Split the pytest parametrization across two parts to make it shorter.

cmarmo · 2020-08-25T12:42:21Z

It seems to me that the codecov failure is not related to the PR. @thomasjpfan, two approvals: time to merge? :) Thanks!

…-learn#15636)

rth reviewed Nov 15, 2019

View reviewed changes

david-cortes force-pushed the master branch from 4bbb6ef to 49cdfc1 Compare November 20, 2019 18:41

david-cortes force-pushed the master branch from 49cdfc1 to ffeb54a Compare December 13, 2019 09:31

david-cortes force-pushed the master branch 2 times, most recently from 4ee0ff2 to 290d7ac Compare December 13, 2019 09:57

adrinjalali reviewed Jan 2, 2020

View reviewed changes

adrinjalali approved these changes Jan 6, 2020

View reviewed changes

jnothman reviewed Jan 9, 2020

View reviewed changes

NicolasHug reviewed Jan 11, 2020

View reviewed changes

david-cortes force-pushed the master branch from 7b43c8f to 87cc704 Compare January 11, 2020 21:16

github-actions Bot added the module:impute label Mar 2, 2020

cmarmo added the Waiting for Reviewer label Jul 10, 2020

david-cortes force-pushed the master branch from 87cc704 to 4e2604c Compare July 12, 2020 18:43

david-cortes added 6 commits August 16, 2020 21:35

Mention changes to random_state in IterativeImputer in whats_new, add…

2961105

… whitespace to its test

Remove spaces from empty line for linter checks

6a8a3bb

shorten description in whats_new, rename parameters in tests

982350d

better explanation of the random_state logic for imputer

a97738b

move description about PR to new version

872336c

compare states of RandomState objects

162088c

david-cortes force-pushed the master branch from aabc3f8 to 162088c Compare August 16, 2020 18:39

thomasjpfan reviewed Aug 16, 2020

View reviewed changes

changes after review suggestions

abee3ac

avoid setting estimator's random state at all

6565ada

remove unneeded imports after removing test

ab76d6d

thomasjpfan reviewed Aug 17, 2020

View reviewed changes

adrinjalali approved these changes Aug 21, 2020

View reviewed changes

NicolasHug approved these changes Aug 21, 2020

View reviewed changes

david-cortes added 2 commits August 22, 2020 11:05

change whats_new entry for iterative imputer to 'fix'

270f7c0

test that iterative imputer doesn't set the estimator's random state

1a50b30

thomasjpfan reviewed Aug 22, 2020

View reviewed changes

shorten parametrization in pytest for random states

16a12a6

cmarmo removed the Waiting for Reviewer label Aug 25, 2020

thomasjpfan approved these changes Aug 25, 2020

View reviewed changes

thomasjpfan changed the title ~~[ENH] Pass integer random_state to estimator in IterativeImputer~~ ENH Do not pass random_state to estimator in IterativeImputer Aug 25, 2020

thomasjpfan merged commit f1fe837 into scikit-learn:master Aug 25, 2020

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

ENH Do not pass random_state to estimator in IterativeImputer (scikit…

785d701

…-learn#15636)

ghost mentioned this pull request Nov 19, 2023

[MRG] Make random_state test for IterativeImputer behave as expected #27807

Closed

	if self._estimator.random_state is None:
	if self._estimator.random_state is None and self.random_state_ is not None:

	:pr: `15636` by :user:`David Cortes` <david-cortes>.
	:pr: `15636` by :user:`David Cortes <david-cortes>`.

		- \|Enhancement\| :class:`impute.IterativeImputer` Will set the estimator's
		`random_state` to the same `random_state` that was passed to the imputer.

		``sample_posterior`` is True. If the estimator has a `random_state`,
		but it's set to None, will set it to this value too. Use an integer



		@pytest.mark.parametrize(
		"rs_imputer, rs_estimator, expect_in_fit",

	"rs_imputer, rs_estimator, expect_in_fit",
	"rs_imputer, rs_estimator, rs_expect",

Conversation

david-cortes commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth 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!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Nov 17, 2019 via email

Uh oh!

david-cortes commented Nov 20, 2019

Uh oh!

adrinjalali commented Dec 12, 2019

Uh oh!

NicolasHug commented Dec 12, 2019

Uh oh!

david-cortes commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Dec 12, 2019

Uh oh!

david-cortes commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug commented Dec 13, 2019

Uh oh!

adrinjalali commented Dec 14, 2019

Uh oh!

adrinjalali commented Dec 14, 2019

Uh oh!

david-cortes commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-cortes commented Jan 4, 2020

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-cortes commented Jan 6, 2020

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!

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!

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!

david-cortes commented Jan 10, 2020

Uh oh!

david-cortes commented Nov 15, 2019 •

edited

Loading

david-cortes commented Dec 12, 2019 •

edited

Loading

david-cortes commented Dec 13, 2019 •

edited

Loading

NicolasHug Jan 11, 2020 •

edited

Loading

david-cortes commented Jan 11, 2020 •

edited

Loading