Skip to content

Use custom propensity_learner on R-learner's first fit#937

Merged
jeongyoonlee merged 1 commit into
uber:masterfrom
arpitjain099:fix/rlearner-propensity-learner-first-fit
Jul 9, 2026
Merged

Use custom propensity_learner on R-learner's first fit#937
jeongyoonlee merged 1 commit into
uber:masterfrom
arpitjain099:fix/rlearner-propensity-learner-first-fit

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Proposed changes

The R-learner ignores a user-supplied propensity_learner on the first fit() of a fresh object. In each fit() override the line self.model_p = self.propensity_learner runs after _set_propensity_models(), but _set_propensity_models reads self.model_p (via hasattr) to decide whether to use the custom propensity model or fall back to the default ElasticNetPropensityModel. On a new object model_p doesn't exist yet, so the first fit quietly uses the default and the custom learner is dropped.

Because the attribute persists on the object, a second fit() of the same instance does pick up the custom learner, so the same call gives different nuisance estimates depending on fit order. The fix moves the model_p assignment ahead of the propensity-model fitting in all three affected paths (BaseRLearner, BaseRClassifier, XGBRRegressor).

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)

Test plan

  • Added test_BaseRLearner_uses_custom_propensity_learner_on_first_fit in tests/test_meta_learners.py: it fits a fresh R-learner with a distinctive ElasticNetPropensityModel subclass and asserts the trained propensity_model is that subclass. It fails on master (default ElasticNet is used) and passes with this change.
  • The R-learner test subset (-k "RLearner or RRegressor or RClassifier", 21 tests) passes locally.

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

The R-learner fit() methods assigned self.model_p = self.propensity_learner
after calling _set_propensity_models(), which reads self.model_p to decide
whether to use a user-supplied propensity model. On a fresh object model_p
did not exist yet, so the first fit silently fell back to the default
ElasticNetPropensityModel and ignored propensity_learner. A second fit of the
same object then picked it up, so identical calls produced different nuisance
estimates depending on fit order.

Move the model_p assignment ahead of the propensity-model fitting in
BaseRLearner, BaseRClassifier, and XGBRRegressor, and add a regression test.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

@jeongyoonlee jeongyoonlee 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 added the bug Something isn't working label Jul 9, 2026
@jeongyoonlee jeongyoonlee merged commit 78176f8 into uber:master Jul 9, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants