Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading warning for multiple treatments #786

Open
FlorianNachtigall opened this issue Jun 23, 2023 · 2 comments
Open

Misleading warning for multiple treatments #786

FlorianNachtigall opened this issue Jun 23, 2023 · 2 comments

Comments

@FlorianNachtigall
Copy link

If multiple treatments are specified, a warning is given if a scalar is provided as treatment value for the effect estimation, e.g. est.effect(X=X, T0=0, T1=1).

if (ndim(T) == 0) and self._d_t_in and self._d_t_in[0] > 1:
warn("A scalar was specified but there are multiple treatments; "
"the same value will be used for each treatment. Consider specifying"
"all treatments, or using the const_marginal_effect method.")

However, since this check currently includes the base treatment T0, for which the default is 0, a warning is emitted even if the base treatment is not specified. As a user, this may be misleading because one suspects the provided target treatment T1 to be of undesired shape. (At least for me, it took me quite a while to figure out that the warning was not related to T1.)

Option 1
I would suggest to suppress the warning if the base treatment has the default value (see PR #784).

Option 2
The alternative that would involve less lines of code would be adding a T != 0 check before emitting the warning (see PR #785), i.e.:

if (ndim(T) == 0) and self._d_t_in and self._d_t_in[0] > 1 and T != 0:

Assuming that there are no reasonable cases where the T1 is 0, this should work as well. Yet, I slightly favor the first approach because the second one is highly implicit.

Thanks!

@fverac
Copy link
Collaborator

fverac commented Jul 19, 2023

Thanks for your feedback and suggested solutions.

I feel the warning is still valuable even when T0 and T1 are left as default, but for your case would the warning be clearer if it specified which argument (e.g. T0) it was referring to?

@FlorianNachtigall
Copy link
Author

Yes, that would make it clearer. But I think implementing this is not as straightforward as it seems. Since _expand_treatments takes a variable number of arguments (*Ts), it would be difficult to distinguish between base and target treatment in the error message (without overloading the function with unnecessary external context). Specifying only the index of the scalar T might, I fear, be just as confusing for users.

In general, I'm still a bit on the fence about whether a warning is ideal for default arguments. In a perfect world, I would say that either the default is reasonable and no warning is needed, or there should be no default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants