Fix #1029: Thread random_state to BaseRLearner bootstraps - #1032
Conversation
jeongyoonlee
left a comment
There was a problem hiding this comment.
np.random.default_rng(self.random_state) (rlearner.py:315, :420) rejects a RandomState instance, which rlearner.py:62 documents as supported:
BaseRRegressor(learner=LinearRegression(), random_state=np.random.RandomState(42)) \
.fit_predict(X, y=y, treatment=t, return_ci=True)
# TypeError: SeedSequence expects int or sequence of ints for entropy not RandomState(MT19937)Same for estimate_ate(..., bootstrap_ci=True). Both work on master, so this is a regression.
Please use sklearn.utils.check_random_state(self.random_state) instead — it normalizes None/int/RandomState to a RandomState, which has .choice(), so the rest of each hunk is unchanged. That is also what the tree side already does (match.py:143, causalforest.py:482, upliftforest.py:66), and it matches sklearn's guidance: Controlling randomness recommends passing RandomState instances to estimators, and sklearn does not accept numpy Generator at all.
The new test only passes an int, so the instance path stays uncovered — please add it.
a5bed84 to
b55e864
Compare
… regression test
b55e864 to
f36d328
Compare
|
Done. |
Proposed changes
Fixes #1029.
BaseRLearnerbootstrap confidence intervals could depend on unrelated global NumPy RNG state even whenrandom_statewas provided.This change:
BaseRLearner.random_state.fit_predict()andestimate_ate().The metric functions are unchanged because they already use their provided
random_statefor bootstrap sampling.Before & After
Test
Types of changes
Checklist
Further comments
The regression test verifies that identical seeded
BaseRRegressorruns produce identical confidence intervals regardless of unrelated global NumPy RNG consumption.