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

Make T0 and T1 on effect keyword-only to avoid confusion #79

Closed
kbattocchi opened this issue Jun 28, 2019 · 5 comments
Closed

Make T0 and T1 on effect keyword-only to avoid confusion #79

kbattocchi opened this issue Jun 28, 2019 · 5 comments
Assignees

Comments

@kbattocchi
Copy link
Collaborator

When using inference, it's somewhat ambiguous whether a call like

est.effect_interval(X, low, high)

uses low and high as the optional treatments or as the lower and upper bounds of the confidence interval (the first interpretation is correct).

This confusion could be avoided by making T0 and T1 into keyword-only arguments, so that the user can call

est.effect_interval(X)
est.effect_interval(X, T0=low, T1=high)
est.effect_interval(X, lower=low, upper=high)

but not

est.effect_interval(X, low, high)

This would be a breaking change, but the safety benefits probably make it worthwhile anyway.

@kbattocchi kbattocchi self-assigned this Jun 28, 2019
@moprescu
Copy link
Contributor

How does that influence the effect function? I.e. would the user still be able to call effect(x) and effect(x, 0, 1)?

@vasilismsr
Copy link
Contributor

it will have to be effect(x, T0=0, T1=1)

@kbattocchi
Copy link
Collaborator Author

kbattocchi commented Jun 28, 2019

@moprescu, that is a good question.

The simplest approach would be to apply the same rule to effect itself, so that you could call effect(x) and effect(x, T0=0, T1=1) but not effect(x, 0, 1), although this would be a breaking change which otherwise seems unnecessary (since there's no ambiguity here).

Unfortunately, if we want the bootstrap wrapper to remain generic, then it's hard to see how to fix effect_interval without changing effect...

@kbattocchi
Copy link
Collaborator Author

@moprescu The changes in #75 will make these arguments keyword-only for effect_interval while leaving effect untouched by explicitly creating an effect_interval method on BaseCateEstimator.

@vasilismsr
Copy link
Contributor

#75 addresses this

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

3 participants