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

Vasilis/drate #391

Merged
merged 36 commits into from
Mar 11, 2021
Merged

Vasilis/drate #391

merged 36 commits into from
Mar 11, 2021

Conversation

vsyrgkanis
Copy link
Collaborator

No description provided.

@vsyrgkanis vsyrgkanis marked this pull request as ready for review February 5, 2021 20:14
@vsyrgkanis vsyrgkanis requested a review from kbattocchi March 7, 2021 19:25
@vsyrgkanis vsyrgkanis requested review from moprescu and heimengqi March 7, 2021 19:25
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.

Please make sure that the new logic is well covered by tests.

Comment on lines +1129 to +1130
raise AttributeError("`oob_predict` is not available when "
"the estimator was fitted with `warm_start=True`")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be mentioned in the method docstring? What about the warm_start part of the init docstring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added it in docstring. Also added it in the warm_strt docstring. I also removed warm_Start from causalForestDML. It makes no sense there as there is no way for the final model to be refit to new data! So that only is a weird option to have.

econml/dml/causal_forest.py Outdated Show resolved Hide resolved
econml/dml/causal_forest.py Outdated Show resolved Hide resolved
if not self._drate:
raise AttributeError("Doubly Robust ATE calculation on training data "
"is available only when `drate=True`!")
return self._ate
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason the coverage report shows this line (and the corresponding ate_stderr_ one as uncovered... Does that make sense?

over the training data and with a doubly robust correction.
Available only when `discrete_treatment=True` and `oob=True`.
"""
return NormalInferenceResults(d_t=self._d_t[0] if self._d_t else 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and the subsequent properties are also showing as uncovered by tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is weird! This should be covered!

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.

LGTM

@vsyrgkanis vsyrgkanis merged commit 41ffb03 into master Mar 11, 2021
@vsyrgkanis vsyrgkanis deleted the vasilis/drate branch March 11, 2021 22:06
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