Skip to content

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.

# =============================================================================


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.

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.

@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.

@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