Skip to content

Cap default LGBM num_leaves to fold/arm sample size in cate_scoring - #1038

Open
aman-coder03 wants to merge 1 commit into
uber:masterfrom
aman-coder03:fix/cate-scoring-num-leaves
Open

Cap default LGBM num_leaves to fold/arm sample size in cate_scoring#1038
aman-coder03 wants to merge 1 commit into
uber:masterfrom
aman-coder03:fix/cate-scoring-num-leaves

Conversation

@aman-coder03

Copy link
Copy Markdown
Contributor

Proposed changes

the default learner in compute_dr_pseudo_outcomes(), dr_score(), and plug_in_t_score() was a fixed LGBMRegressor(num_leaves=64, ...), but these functions fit per-fold and per-arm (10 fits per call with the default n_folds=5), each on a fraction of the data. 64 leaves is often way more than a small slice can support, which just triggers "No further splits with positive gain" warnings instead of helping.

swapped the fixed default for a sentinel that builds a learner sized to however many rows each fold/arm actually has, capped at 64 leaves like before. Also added a docstring note pointing people at pseudo_outcome_col when comparing multiple models, since that avoids refitting the nuisance models every time

fixes #1037

Types of changes

What types of changes does your code introduce to CausalML?
Put an x in the boxes that apply

  • 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

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • 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

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc. This PR template is adopted from appium.

@su-jin1425

Copy link
Copy Markdown
Contributor

I can see that AI was used to work on this issue. I recommend using AI to plan and reason through the problem first, and then implementing and reviewing the solution carefully. Avoid asking AI to directly solve the whole issue, as this can lead to unnecessary changes and make the PR harder to review.

Also, this PR adds quite a lot of code and documentation for a relatively small issue. Please consider simplifying the changes and keeping the PR focused on the num_leaves problem. It would also be good to add a test that directly verifies the expected num_leaves value is used for each fold, rather than only checking that the final scores are finite.

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.

cate_scoring default learner is oversized for its own per-fold, per-arm call pattern

2 participants