[Repo Assist] fix(iv): raise ValueError when instrument has fewer than 2 distinct values#1675
Conversation
…alues Before this change, passing an instrument column with only one unique value silently produced NaN via np.mean([]) when the Wald estimator hard-codes instrument == 1 / instrument == 0. A constant instrument has no statistical variation and cannot identify a causal effect, so a clear ValueError is now raised before the estimation logic runs. This also prevents the IndexError that would occur with the fix in contributor PR #1673 (which uses unique_values[1]) when the instrument has only one unique value. Adds a regression test: test_iv_estimator_raises_on_constant_instrument. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
Commit pushed:
|
* perf(gcm): vectorise inner loops in marginal_expectation Replace two O(batch_size) Python loops with vectorised NumPy operations in marginal_expectation(): 1. Input-filling loop: previously assigned baseline values one row at a time with a Python loop; replaced with a single np.repeat() call that broadcasts all baseline rows in one shot. 2. Result-aggregation loop: previously called np.mean() and sliced predictions inside a Python loop; replaced with a reshape + .mean(axis=1) that computes all batch means in a single NumPy call, and a list-slice assignment to store results. Both changes are semantically equivalent to the original code and handle scalar (1-D) as well as multi-output (2-D) prediction functions correctly. marginal_expectation() is the hot path inside Shapley-based feature relevance, arrow strength, and intrinsic causal influence computations, where it is evaluated once per coalition subset. Removing the Python-level loop overhead is most significant when baseline_samples is large (typical default: 500 samples with batch_size=100, giving 5 batches × 100 Python iterations → now 5 batches × 1 NumPy op each). All 19 test_stats tests and all 21 test_feature_relevance / test_intrinsic_influence / test_arrow_strength tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * build(deps-dev): bump soupsieve from 2.7 to 2.8.4 (#1670) * build(deps-dev): bump mistune from 3.2.1 to 3.3.0 (#1671) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Patrick Blöbaum <51325689+bloebp@users.noreply.github.com> --------- Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Patrick Blöbaum <51325689+bloebp@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Patrick Blöbaum <51325689+bloebp@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Commit pushed:
|
|
Commit pushed:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR improves IV estimation robustness by raising a clear ValueError when the instrument is constant (has <2 distinct values), and adds a regression test to ensure this behavior. It also includes a separate vectorization change in marginal_expectation that is not described in the PR metadata.
Changes:
- Raise
ValueErrorinInstrumentalVariableEstimator.estimate_effect()when the instrument has fewer than 2 unique values. - Add a unit test asserting the estimator raises on a constant instrument.
- Vectorize batching/aggregation logic in
dowhy/gcm/stats.py:marginal_expectation().
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/causal_estimators/test_instrumental_variable_estimator.py | Adds regression test for constant-instrument validation (and new imports). |
| dowhy/causal_estimators/instrumental_variable_estimator.py | Adds early validation + user-facing error for <2 unique instrument values. |
| dowhy/gcm/stats.py | Refactors marginal_expectation() to reduce Python loops via vectorized reshape/broadcast. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # baseline_noise_samples.shape[0] * feature_samples.shape[0]. Here, we reduce it to | ||
| # batch_size * feature_samples.shape[0]. If the batch_size would be set 1, then each baseline_noise_samples is | ||
| # evaluated one by one in a for-loop. | ||
| n_f = feature_samples.shape[0] |
| # Vectorised: broadcast each baseline row across its n_f feature-sample rows in one numpy operation, | ||
| # replacing the per-sample Python loop that previously did this assignment one row at a time. |
| # Vectorised aggregation: reshape into (adjusted_batch_size, n_f, ...) and reduce in one numpy call, | ||
| # replacing the per-sample Python loop that previously sliced and averaged one sample at a time. |
| result[offset : offset + adjusted_batch_size] = list(pred_batched.mean(axis=1)) | ||
| else: | ||
| # Return all n_f prediction rows for each baseline sample (no averaging). | ||
| result[offset : offset + adjusted_batch_size] = list(pred_batched) |
| "Instrument variable '{}' must have at least 2 distinct values for IV estimation, " | ||
| "but only {} unique value was found. A constant instrument has no statistical " | ||
| "variation and cannot identify the causal effect.".format( | ||
| self.estimating_instrument_names[0], num_unique_values | ||
| ) |
| @@ -1,8 +1,11 @@ | |||
| import itertools | |||
|
|
|||
| import numpy as np | |||
🤖 This PR was created by Repo Assist, an automated AI assistant.
Root Cause
InstrumentalVariableEstimator.estimate_effect()uses the Wald estimator whenever the instrument has ≤ 2 distinct values. However, when the instrument is constant (only 1 unique value), the hard-coded masksinstrument == 1andinstrument == 0produce one empty group, sonp.mean([])silently returnsNaN. The user gets aNaNestimate with no diagnostic information.This is also relevant to contributor PR #1673, which changes the Wald branch to use
unique_values[1]— ifnum_unique_values == 1that would raise anIndexErrorinstead of a helpful message.Fix
Add a validation check immediately after computing
num_unique_values, before entering either estimator path:A constant instrument is never a valid instrument (it violates the relevance assumption), so raising immediately is the correct behaviour.
Trade-offs
IndexErrorthat Fix silent NaN in IV Wald estimator for non-{0,1} binary instruments #1673'sunique_values[1]unpack would produce for a 1-value instrument.Tests
Added
test_iv_estimator_raises_on_constant_instrumenttotests/causal_estimators/test_instrumental_variable_estimator.py:0.0.model.estimate_effect(..., method_name="iv.instrumental_variable")raisesValueErrorwith message matching"at least 2 distinct values".Test Status
black --check✅ (files unchanged by formatter)isort --check✅ (files unchanged by formatter)test_iv_estimator_raises_on_constant_instrument✅ passesTestInstrumentalVariableEstimator.test_average_treatment_effect✅ all 2 cases pass