Skip to content

Stop the #854 shim warning on causalml's own internal fit calls - #988

Merged
jeongyoonlee merged 1 commit into
masterfrom
fix/internal-positional-fit-calls
Aug 1, 2026
Merged

Stop the #854 shim warning on causalml's own internal fit calls#988
jeongyoonlee merged 1 commit into
masterfrom
fix/internal-positional-fit-calls

Conversation

@jeongyoonlee

Copy link
Copy Markdown
Collaborator

Proposed changes

Follow-up to #975. The deprecation shim suppresses re-entrant warnings with a flag stored on the instance, so it covers self.fitself.predict nesting but not the forest → tree hop: the flag is set on the forest while the warning fires on each tree it fits.

The result is a warning the caller cannot act on or silence:

>>> CausalRandomForestRegressor(n_estimators=2).fit(X=X, y=y, treatment=treatment)
FutureWarning: Passing `treatment` and/or `y` to fit() by position is deprecated ...
FutureWarning: Passing `treatment` and/or `y` to fit() by position is deprecated ...

That call is all-keyword — written exactly the way the warning asks for — and it emits one FutureWarning per tree: 100 at the default n_estimators. UpliftRandomForestClassifier behaves the same way (default 10).

This makes the forests pass y and treatment to their trees by keyword. It is the same migration #982 applies to tests/, applied to the library's own call sites — the epic (#980) has no ticket covering those.

Call sites changed

File
causalml/inference/tree/causal/causalforest.py:136 subsample / bootstrap branch
causalml/inference/tree/causal/causalforest.py:145 non-subsample branch
causalml/inference/tree/_uplift/upliftforest.py:73 bagged uplift tree fit

Verification

An empirical probe over eleven public entry points (both forests, both trees, the S/T/X/R/DR learners, estimate_ate, fit_predict), all called with keywords, attributes each warning to its originating library line via stacklevel=2: 2 offending sites before, 0 after.

  • tests/test_fit_arg_order.py: 54 passed.
  • tests/test_causal_trees.py tests/test_uplift_trees.py tests/test_uplift_trees_kernel.py tests/test_serialization_extended.py: 135 passed.
  • Both new regression tests fail with the library change reverted and pass with it, so they pin the fix rather than passing vacuously.
  • black --check clean.

Left alone, deliberately

The same-instance super().fit(X, treatment, y, ...) calls in upliftforest.py:342, uplifttree.py:567 and rlearner.py:913. The guard already covers them and they warn nobody today. They do need to change under #985 — once the signature order flips, treatment silently lands in y.

Note on why this was not caught

The repo sets no filterwarnings anywhere, so FutureWarning is never an error and the affected tests pass on a normal run. A deprecation shim without a warnings-as-errors lane is untested by construction; worth pairing with the CI work in #977.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

🤖 Generated with Claude Code

The deprecation shim suppresses re-entrant warnings with a flag stored on
the instance, so it covers self.fit -> self.predict nesting but not the
forest -> tree hop: the flag is set on the forest while the warning fires
on each tree it fits.

CausalRandomForestRegressor.fit(X=X, y=y, treatment=treatment) — an
all-keyword call, written exactly the way the warning asks for — therefore
emitted one FutureWarning per tree, 100 of them at the default
n_estimators, telling the caller to do what they had already done. Nothing
the caller could change would silence it. UpliftRandomForestClassifier had
the same behavior (default 10).

Make the forests pass y and treatment to their trees by keyword. This is
the same migration #982 applies to tests/, applied to the library's own
call sites; the epic has no ticket covering those.

An empirical probe over eleven public entry points (both forests, both
trees, the S/T/X/R/DR learners, estimate_ate and fit_predict), all called
with keywords, goes from two offending sites to zero. The two regression
tests fail without the change.

The same-instance super().fit(X, treatment, y, ...) calls in
upliftforest.py, uplifttree.py and rlearner.py are left alone: the guard
already covers them and they warn nobody today. They do need to change
when #985 flips the signature order, or treatment will silently land in y.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeongyoonlee
jeongyoonlee marked this pull request as ready for review July 31, 2026 11:06

@paullo0106 paullo0106 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jeongyoonlee
jeongyoonlee merged commit 18c92dc into master Aug 1, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants