Skip to content

[Repo Assist] fix: raise ValueError in estimate_effect() when identified estimand is None#1554

Draft
github-actions[bot] wants to merge 1 commit into
mainfrom
repo-assist/fix-issue-1551-estimate-effect-none-estimand-20260530-6a45dba3950d827c
Draft

[Repo Assist] fix: raise ValueError in estimate_effect() when identified estimand is None#1554
github-actions[bot] wants to merge 1 commit into
mainfrom
repo-assist/fix-issue-1551-estimate-effect-none-estimand-20260530-6a45dba3950d827c

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This is an automated PR from Repo Assist, an AI assistant for this repository.

Closes #1551

Root Cause

estimate_effect() in causal_estimator.py (lines 746–750) had a silent failure path: when identified_estimand.estimands[identifier_name] is None (which happens when identification fails — e.g. calling iv.instrumental_variable but the graph contains no instruments), the function logged a logger.error message and returned a CausalEstimate(value=None). Callers received no programmatic signal that something went wrong.

# Before (silent failure)
elif identified_estimand.estimands[identifier_name] is None:
    logger.error("No valid identified estimand available.")
    return CausalEstimate(None, None, None, None, None, None, ...)

Fix

Replace the silent return with a ValueError that includes a descriptive message:

# After (explicit failure)
elif identified_estimand.estimands[identifier_name] is None:
    raise ValueError(
        f"No valid identified estimand for '{identifier_name}'. "
        "Ensure that the identification step succeeded for this estimator method "
        "(e.g. the graph must contain valid instruments for 'iv.instrumental_variable')."
    )

Why ValueError and not a different exception? The call is invalid because the chosen method cannot be applied given the graph — the identifier name ("iv") does not correspond to a valid estimand. This is a value/argument error, consistent with how InstrumentalVariableEstimator.fit() and TwoStageRegressionEstimator.fit() handle similar invalid configurations.

Note: This is different from the no_directed_path branch just above, which correctly returns CausalEstimate(value=0) because "no causal effect" is a meaningful, valid result. A None estimand means identification itself failed.

Relationship to #1521 / PR #1520

PR #1520 (by @abhiprd2000, who also filed #1551) fixes the iv branch of TwoStageRegressionEstimator.fit() to raise ValueError when no instruments are present. This PR fixes the same class of issue one level up in the call stack — in the shared estimate_effect() function that wraps all estimators — so that all estimators benefit regardless of their internal implementation.

Changes

  • dowhy/causal_estimator.py: replace silent return CausalEstimate(None) with raise ValueError (3 lines changed)
  • tests/causal_estimators/test_instrumental_variable_estimator.py: add regression test test_estimate_effect_raises_when_iv_estimand_is_none that exercises the exact scenario from the issue report

Test Status

⚠️ CI requires maintainer approval to run. The fix was verified by code inspection:

  • The changed path is exercised when identified_estimand.estimands["iv"] is None, which occurs when num_instruments=0 in linear_dataset (confirmed by tracing auto_identifier.py:291)
  • The new test explicitly asserts this precondition before calling estimate_effect()
  • Syntax verified: python3 -m py_compile passes on both changed files
  • Line lengths verified: all within the 120-char limit
  • No new dependencies introduced

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@11c9a2c442e519ff2b427bf58679f5a525353f76

…s None

Previously, calling estimate_effect() with a method whose estimand is None
(e.g. 'iv.instrumental_variable' when no instruments are in the graph) would
silently return a CausalEstimate with value=None, with only a logger.error
message. Callers had no programmatic indication that identification failed.

This replaces the silent return with a ValueError that includes a descriptive
message explaining which identifier failed and why, matching the contract
expected by users (per issue #1551).

Closes #1551

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation bug Something isn't working repo-assist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

estimate_effect() silently returns None when no valid estimand is identified

0 participants