[Repo Assist] fix: handle list-form treatment_variable in estimate_effect_naive#1461
Conversation
estimate_effect_naive() failed with a ValueError when treatment_variable or outcome_variable were lists (as returned by parse_state), because data[[list]] == 1 produces a 2D boolean DataFrame that cannot be used as a .loc index key. Also fix hardcoded treatment_value=1 / control_value=0: now uses self._treatment_value and self._control_value (set by update_input()) so non-binary treatments work correctly. Fixes #416 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes estimate_effect_naive() to correctly handle list-form treatment_variable / outcome_variable and to respect caller-provided treatment/control values (per #416).
Changes:
- Extract scalar treatment/outcome column names from list-form variables to avoid 2D boolean indexing.
- Use
self._treatment_value/self._control_value(with 1/0 fallback) instead of hardcoded values.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Emre Kıcıman <emrek@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Emre Kıcıman <emrek@microsoft.com>
…ive-ca2d9bc3d2269e11
|
🤖 This is an automated note from Repo Assist. This PR is superseded by #1553 and can be safely closed. PR #1553 ("[Repo Assist] fix: estimate_effect_naive handles list treatment_variable and uses actual treatment/control values") is a newer, more comprehensive fix for issue #416 that:
Maintainers: please close this PR and review #1553 instead.
|
🤖 This PR was created by Repo Assist, an automated AI assistant. Please review carefully before merging.
Closes #416
Root Cause
estimate_effect_naive()inCausalEstimatorhad two related bugs:List indexing:
self._target_estimand.treatment_variableandoutcome_variableare always lists (returned byparse_state()). The old code diddata[list_of_names] == 1, which produces a 2D boolean DataFrame. Using that as a.locindex key raises:Hardcoded treatment/control values: The method used
== 1/== 0, ignoring the actualtreatment_value/control_valuethat the caller passed toestimate_effect(). For non-binary or custom-valued treatments this returned a wrong or empty estimate.Fix
treatment_variable[0]), falling back gracefully if already a scalar.self._treatment_value/self._control_value(set byupdate_input()/estimate_effect()) instead of the hardcoded constants. Falls back to1/0if not yet set.The fix is surgical — 4 changed lines in
estimate_effect_naive()only.Test Status
Code quality checks:
black --checkpassed (no formatting changes needed)Manual verification: The logic was verified by code inspection:
treatment_variable[0]correctly extracts the single column namedata[scalar_name] == valueproduces a 1D boolean Series (correct for.loc)self._treatment_valueis set inupdate_input()which is called by all concrete estimators'estimate_effect()beforeevaluate_effect_strengthis called