Skip to content

Add Policy Tree class for learning policies based on IRM #212

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

Merged
merged 19 commits into from
Sep 8, 2023

Conversation

OliverSchacht
Copy link
Member

@OliverSchacht OliverSchacht commented Sep 1, 2023

With this PR I would like to add a Policy Tree implemenation.

Description

  • Adding a DoubleMLPolicyTree class that fits a weighted classification of arbitrary covariates on an orthogonal basis.
  • Adding a policy_tree() method to the IRM model, that fits and returns a Policy Tree based on the ATE score.
  • Adding unit tests for both the new class and the new method.

Reference to Issues or PRs

Development of new features.

PR Checklist

  • The title of the pull request summarizes the changes made.
  • The PR contains a detailed description of all changes and additions.
  • References to related issues or PRs are added.
  • The code passes all (unit) tests.
  • Enhancements or new feature are equipped with unit tests.
  • The changes adhere to the PEP8 standards.

@OliverSchacht OliverSchacht marked this pull request as draft September 1, 2023 17:29
@OliverSchacht OliverSchacht marked this pull request as ready for review September 1, 2023 19:58
Copy link
Member

Choose a reason for hiding this comment

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

  • remove the sentence "However, they ..." from the depth documentation. This should be part of the docs.
  • "learnt" to "learned" ? Maybe easier with US English
  • Line 488. groups should be x_vars. Maybe rename x_vars to covariates or features.
  • is min_samples_leaf=5 sufficient? I think, we should increase it slightly.
  • Line 486. Change "Default is 2" to "Default is 2."

Copy link
Member

Choose a reason for hiding this comment

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

  • See documentation comments in double_ml_irm.py

Copy link
Member

Choose a reason for hiding this comment

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

Pls remove

Copy link
Member

Choose a reason for hiding this comment

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

What are you testing specifically. Would this not be better at test_doubleml_return_types ?

@OliverSchacht
Copy link
Member Author

Thank you for your review @SvenKlaassen
I adapted the following changes:

  • rename x_vars to features
  • set min_samples_leaf to 8
  • move policytree return type test from test_irm.py to test_doubleml_return_types.py
  • fixed minor spelling / documentation issues you mentioned

@SvenKlaassen SvenKlaassen merged commit 0851e88 into main Sep 8, 2023
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.

2 participants