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/policy #377

Merged
merged 49 commits into from
Mar 22, 2021
Merged

Vasilis/policy #377

merged 49 commits into from
Mar 22, 2021

Conversation

vsyrgkanis
Copy link
Collaborator

Added policy module.

Enabled policy interpreter for multiple treatments using a custom policy tree.

Add doubly robust forest policy learner.

@vsyrgkanis vsyrgkanis marked this pull request as ready for review March 17, 2021 16:15
Copy link
Contributor

@moprescu moprescu left a comment

Choose a reason for hiding this comment

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

Looked mainly at the policy part and the notebooks. Everything looks good, some minor comments and one metadata fix required.

econml/policy/_drlearner.py Show resolved Hide resolved
econml/policy/_drlearner.py Show resolved Hide resolved
econml/policy/_drlearner.py Show resolved Hide resolved
econml/policy/forest/_tree.py Outdated Show resolved Hide resolved
econml/policy/forest/_forest.py Outdated Show resolved Hide resolved
# =============================================================================


class PolicyForest(BaseEnsemble, metaclass=ABCMeta):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, a lot of similarities with the GRF forest, except that the GRF has a lot more auxiliary functions (about 2/3 of the code). Most of the components here appear in the GRF forest as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried for this one, but there was so much different small details that I thought it was ok for the forest.

I merged them in the tree, but left the forests separate.

Copy link
Contributor

@heimengqi heimengqi 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! Just comment on a few places need to add/update docstring.

econml/cate_interpreter/_interpreters.py Show resolved Hide resolved
econml/cate_interpreter/_interpreters.py Outdated Show resolved Hide resolved
econml/policy/forest/_forest.py Outdated Show resolved Hide resolved
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.

Mostly looks good, added a few small comments.

econml/_tree_exporter.py Show resolved Hide resolved
econml/policy/__init__.py Show resolved Hide resolved
econml/tests/test_cate_interpreter.py Show resolved Hide resolved
@kbattocchi
Copy link
Collaborator

Shouldn't the new policy files also be added to our documentation?

@vsyrgkanis
Copy link
Collaborator Author

Shouldn't the new policy files also be added to our documentation?

done! Added the autosummary for the policy moduel

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. I added a minor suggestion on the test.

Copy link
Contributor

@heimengqi heimengqi left a comment

Choose a reason for hiding this comment

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

I go over the docstring, looks great! Only in a few places miss max_depth.

econml/_tree_exporter.py Show resolved Hide resolved
econml/_tree_exporter.py Show resolved Hide resolved
econml/policy/_drlearner.py Show resolved Hide resolved
econml/policy/_drlearner.py Show resolved Hide resolved
@vsyrgkanis vsyrgkanis merged commit 98b2bf3 into master Mar 22, 2021
@vsyrgkanis vsyrgkanis deleted the vasilis/policy branch March 22, 2021 22:56
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.

5 participants