Skip to content

Conversation

@mohsinm-dev
Copy link
Contributor

Summary

Fixes issue #848 where the estimation_sample_size parameter was not being passed to individual trees in UpliftRandomForestClassifier.

Changes

  • Added parameter storage in UpliftRandomForestClassifier.__init__ (line 2430)
  • Added parameter passing to individual UpliftTreeClassifier instances during forest creation (line 2481)

Root Cause

The constructor accepted the estimation_sample_size parameter but failed to:

  1. Store it as an instance variable (self.estimation_sample_size = estimation_sample_size)
  2. Pass it to individual UpliftTreeClassifier instances during forest creation

Impact

This fix enables users to properly control honest splitting behavior in random forests when honesty=True. Previously, all trees defaulted to 0.5 regardless of user specification.

Testing

  • Verified existing test suite passes (102/103 tests pass, 1 unrelated GraphViz failure)
  • Confirmed no breaking changes or regressions
  • Tested with multiple estimation_sample_size values (0.1-0.9)
  • Verified default behavior preservation (0.5 when not specified)

Files Changed

  • causalml/inference/tree/uplift.pyx (2 line addition)

@jeongyoonlee jeongyoonlee added the bug Something isn't working label Sep 26, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where the estimation_sample_size parameter was not properly passed to individual trees in UpliftRandomForestClassifier. The fix enables correct honest splitting behavior when honesty=True.

  • Stores the estimation_sample_size parameter as an instance variable
  • Passes the parameter to individual UpliftTreeClassifier instances during forest creation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@jeongyoonlee jeongyoonlee left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution, @mohsinm-dev!

@jeongyoonlee jeongyoonlee merged commit 6f4ec71 into uber:master Sep 26, 2025
9 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