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

Correct default value of alphas #476

Merged
merged 11 commits into from
Aug 9, 2021
Merged

Correct default value of alphas #476

merged 11 commits into from
Aug 9, 2021

Conversation

mtanghu
Copy link
Contributor

@mtanghu mtanghu commented Jun 1, 2021

The PopulationSummaryResults class should to default to self.alpha when the alpha parameter is not passed. This is not the case currently since some methods use the statement 'alpha = self.alpha if alpha is None else alpha', however alpha's default value in the methods is .1, thus self.alpha will be ignored even if the client does not pass the alpha parameter.

Without the fix ate_interval(), marginal_ate_interval() and const_marginal_ate_interval() all ignore the alpha parameter.

The PopulationSummaryResults class should to default to self.alpha when the alpha parameter is not passed. This is not the case currently since some methods use the statement 'alpha = self.alpha if alpha is None else alpha', however alpha's default value in the methods is .1, thus self.alpha will be ignored even if the client does not pass the alpha parameter.

Without the fix ate_interval(), marginal_ate_interval() and const_marginal_ate_interval() all ignore the alpha parameter.
@heimengqi
Copy link
Contributor

Thanks @mtanghu ! Great catch! I will address that thoroughly in a separate PR.

@heimengqi
Copy link
Contributor

@mtanghu We are happy to have more external contributors. I think your current fix have already partially addressed this issue, and we have the similar issue for value, tol and decimals as well, it would be great if you'd like to implement those changes as well.

The logic will be in the initializer we have some default alpha, value, tol, decimals since call PopulationSummaryResult will output the summary table, and if users still want to call PopulationSummaryResult.conf_int_mean() for instance, by default it will use the same alpha set from initializer, but if they call .conf_int_mean(alpha=bla), it will update the CI with new alpha. In this case, default for alpha, value, tol, decimals in each method has to be None.

@mtanghu
Copy link
Contributor Author

mtanghu commented Jun 4, 2021

@heimengqi I'd love to! I'll do that this weekend!

Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this mostly looks good, but I've added a few extra suggestions (one of which was also made by @heimengqi).

Once those are addressed I'd be happy to approve this.

econml/inference/_inference.py Outdated Show resolved Hide resolved
econml/inference/_inference.py Show resolved Hide resolved
@mtanghu mtanghu marked this pull request as draft June 4, 2021 20:44
@kbattocchi
Copy link
Collaborator

@mtanghu Are you still interested in following this through, or should someone on our team do the remaining work to get this PR cleaned up and then merged?

@mtanghu
Copy link
Contributor Author

mtanghu commented Jul 1, 2021

@kbattocchi Oh yes sorry, I'm definitely interested and just caught up in other work. I should be able to get this PR cleaned up soon!

@ghost
Copy link

ghost commented Jul 6, 2021

CLA assistant check
All CLA requirements met.

@mtanghu mtanghu marked this pull request as ready for review July 7, 2021 04:23
@mtanghu
Copy link
Contributor Author

mtanghu commented Jul 7, 2021

This should be ready to merge, lmk if there's anything you else you'd like me to help with

Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

Removing whitespace to fix linting
@kbattocchi kbattocchi enabled auto-merge (squash) July 9, 2021 19:01
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

Successfully merging this pull request may close these issues.

3 participants