uber / uber/causalml

Normalize meta-learner random_state handling through check_random_state

Open
#1,035 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

refactoring
Dominant language
Python
Stars
6k
Forks
877
PR merge metrics
No merged PRs in 30d

Description

Prerequisite for #1036 — that issue threads this normalization through the S- and X-learner families, so this one should land first.

#1032 introduced check_random_state to rlearner.py (:7, used at :316 and :421). It is now the only meta-learner that does, which leaves two concrete inconsistencies on master.

a. BaseLearner.bootstrap's rng parameter is now documented wrongly. base.py:128 says rng (np.random.Generator, optional), but as of #1032 rlearner.py:316 passes it the RandomState returned by check_random_state. Both work — .choice() exists on each — but the docstring no longer describes what the only caller supplies.

b. base.py still uses the constructor rather than the normalizer. base.py:40 (_fit_bootstrap_clone) and base.py:186 (fit_bootstrap_ensemble) call np.random.RandomState(random_state), which rejects a RandomState instance:

np.random.RandomState(np.random.RandomState(42))
# TypeError: Cannot cast scalar from dtype('O') to dtype('int64') according to the rule 'safe'

base.py:180 documents that parameter as int, optional, so it is self-consistent — but base.py:292 and :365 document random_state (int/RandomState instance/None) for permutation importance, so base.py is not internally consistent either.

The tree side already follows scikit-learn's convention: match.py:143, tree/_tree/_classes.py:233, causalforest.py:482, causal/_tree.py:101, upliftforest.py:66.

scikit-learn's Controlling randomness guidance is to pass RandomState instances to estimators and ints to CV splitters, and scikit-learn does not support numpy Generator at all — check_random_state(np.random.default_rng(42)) raises ValueError on scikit-learn 1.7.0.

Proposed:

  1. Route every meta-learner RNG through check_random_state.
  2. Widen base.py:128 to np.random.Generator or np.random.RandomState.
  3. Widen base.py:180 to int or RandomState once it holds.

No API narrowing — this widens accepted input.

Observed on master (477cd0a).

🤖 Generated with Claude Code

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with base.py at _fit_bootstrap_clone, fit_bootstrap_ensemble, and the documented random_state parameters, then compare the existing handling in rlearner.py and the tree-side files listed in the issue. Trace every meta-learner RNG entry point and run the relevant test suite. Done means all meta-learners use check_random_state and the base.py documentation matches the accepted inputs.

Written by the indexing model from the issue text.

Assessment

Tech stack
numpy, python, scikit-learn
Domain
machine-learning
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.