Skip to content

Conversation

maartenvanhooftds
Copy link
Contributor

@maartenvanhooftds maartenvanhooftds commented Jun 5, 2025

Issue #875

Changes:

  • Fix for Causal Forest not passing discrete_outcome flag to RScorer. As a result, it gets initiated by it's default value False, which is unintended behaviour if we want discrete_outcome=True
  • Added a test that previously failed, but now succeeds. Based on the minimal example provided here

First contribution here, please review critically for any mistakes or inconsistencies!

I see that the CI pipeline awaits approval. I think I did everything that is expected from me, so I'll mark it as ready for review. But perhaps the pipeline flags something.

Signed-off-by: maartenvanhooft <mjgvanhooft@gmail.com>
@maartenvanhooftds maartenvanhooftds marked this pull request as ready for review June 5, 2025 20:50
@kbattocchi kbattocchi requested a review from Copilot June 6, 2025 16:22
Copy link

@Copilot 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 discrete_outcome flag was not forwarded to RScorer in CausalForestDML.tune and adds a minimal test to ensure tuning and fitting succeed when both outcome and treatment are discrete.

  • Pass discrete_outcome into RScorer during tuning.
  • Add a test that runs tune and fit with discrete_outcome=True and discrete_treatment=True.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
econml/dml/causal_forest.py Included discrete_outcome=est.discrete_outcome in RScorer instantiation within tune.
econml/tests/test_dml.py Added test_causal_forest_tune_with_discrete_outcome_and_treatment to verify no errors when both flags are true.
Comments suppressed due to low confidence (1)

econml/tests/test_dml.py:1304

  • [nitpick] The new test does not include any assertions to verify the estimator's behavior. Consider adding checks—for example, assert that the internally inferred outcome dimension (est._d_y) matches the expected number of classes or that calling est.effect(X) returns an array of the correct shape—to ensure discrete flags are honored.
def test_causal_forest_tune_with_discrete_outcome_and_treatment(self):

Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this looks great. Once our checks pass I'll merge it.

@kbattocchi kbattocchi enabled auto-merge (squash) June 6, 2025 16:54
@kbattocchi kbattocchi merged commit 3058cb5 into py-why:main Jun 7, 2025
461 of 474 checks passed
carl-offerfit pushed a commit to carl-offerfit/EconML that referenced this pull request Jul 7, 2025
Signed-off-by: maartenvanhooft <mjgvanhooft@gmail.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
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