fix: raise ValueError when no valid estimand is identified instead of silent None return#1552
fix: raise ValueError when no valid estimand is identified instead of silent None return#1552abhiprd2000 wants to merge 4 commits into
Conversation
…f silent None return Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
|
Hi @emrekiciman, follow-up to your suggestion on #1520. Fix and regression test included. |
Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
|
Hi @emrekiciman, Noticed github-actions bot also filed a draft #1554 for the same fix. Mine includes an end-to-end test via |
There was a problem hiding this comment.
Pull request overview
This PR makes estimate_effect() fail loudly when identification did not produce a valid estimand for the requested identifier, by raising a ValueError instead of returning a CausalEstimate with a None value. This aligns behavior with the expectation that a missing estimand indicates identification failure rather than a valid “no effect” result.
Changes:
- Raise a
ValueError(with identifier name included) whenidentified_estimand.estimands[identifier_name] is None. - Add a regression test covering the missing-estimand case for
iv.instrumental_variable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dowhy/causal_estimator.py |
Replaces the silent CausalEstimate(None, …) return with a logged ValueError when the estimand is missing. |
tests/test_causal_estimator.py |
Adds a regression test asserting estimate_effect() raises ValueError for missing IV estimand. |
💡 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: Abhimanyu Prasad <8292aniarc@gmail.com>
amit-sharma
left a comment
There was a problem hiding this comment.
thanks @abhiprd2000 This looks good
|
@all-contributors add @abhiprd2000 for code |
|
I've put up a pull request to add @abhiprd2000! 🎉 |
Summary
estimate_effect()silently returnsNonewhenidentified_estimand.estimands[identifier_name]isNone, logging an error but not raising. Users get no indication that identification failed.Fix
Replace the silent
return CausalEstimate(None, ...)withraise ValueError(error_msg)including the identifier name for easier debugging.Impact
ValueErrorinstead of silentNoneFixes #1551
Signed-off-by: abhiprd2000