added warning message and test for issue #837 #919
Conversation
econml/_ortho_learner.py
Outdated
| if nuisances[1].sum() == 0: | ||
| raise ValueError( | ||
| """ | ||
| In fitting nuisances, the estimates for E[T|Z,X,W] are identical to E[T|X,W], | ||
| resulting in a situation where the rows will all be weighted to zero. Please | ||
| examine your instrument variable `Z`. | ||
| """ | ||
| ) | ||
|
|
There was a problem hiding this comment.
This isn't the right place to address this issue: it would apply to all OrthoLearner subclasses, including ones like LinearDRLearner that don't exhibit the bug, and the problem is not that the nuisances sum to 0, it's that all values of the treatment residual are 0, which then makes them unsuitable to use as weights here. However, that logic is not specific to NonParamDMLIV, so if you made the check there it wouldn't make sense to refer to E[T|Z,X,W]. Therefore it would probably be better to make the change here where the T residual is generated.
Signed-off-by: Roni Kobrosly <roni.kobrosly@gmail.com>
for more information, see https://pre-commit.ci
|
Does this look better @kbattocchi ? |
Co-authored-by: Keith Battocchi <kbattocchi@gmail.com> Signed-off-by: Roni Kobrosly <roni.kobrosly@gmail.com>
Co-authored-by: Keith Battocchi <kbattocchi@gmail.com> Signed-off-by: Roni Kobrosly <roni.kobrosly@gmail.com>
|
Thank you for the suggestions and your patience @kbattocchi . Accepted your changes. |
|
One of your commits was not signed, so this is not passing the DCO check, see https://github.com/py-why/EconML/pull/919/checks?check_run_id=31527396832 for instructions on how to fix this (under Rebase the Branch). Also it looks like the test that you had originally added has subsequently been removed in another commit - could you please add it back? |
Hi @kbattocchi , could I get your 👀 on this?