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

Added blb inference option to the OrthoForest #214

Merged
merged 3 commits into from
Feb 7, 2020

Conversation

moprescu
Copy link
Contributor

  • Added Bootstrap of Little Bags inference to the ORF classes
  • Added tests and updated notebook
  • Fixed the marginal effect shape when T is a vector
  • Fixed bugs and reorganized class functionality

@moprescu
Copy link
Contributor Author

Also addresses issue #188

* Added Bootstrap of Little Bags inference to the ORF classes
* Added tests and updated notebook
* Fixed the marginal effect shape when T is a vector
* Fixed bugs and reorganized class functionality
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, just a few minor comments.

@@ -363,8 +363,7 @@ and the `ForestLearners Jupyter notebook <https://github.com/microsoft/EconML/bl
>>> est.fit(Y, T, W, W)
<econml.ortho_forest.ContinuousTreatmentOrthoForest object at 0x...>
>>> print(est.effect(W[:2]))
[[1. ]
[1.2]]
[1.00... 1.19...]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, the shape changing here is good because it was wrong before.

Should I be concerned that the values themselves have changed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the shape change is intentional. The values have changed because the trees are partitioned into segments ("little bags") that share data, so the same random seed gives slightly different results now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, so the trees are partitioned even when fit's inference option is None rather than 'blb'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, otherwise we'd have two ways of partitioning the tress and it would be hard to keep track of. It doesn't affect the estimate much since that's done using all of the 2*n_trees leaves. The only difference is which data samples a particular tree uses, i.e. groups slice_len trees subsample from the same n_samples/2 samples but which n_samples/2 are used differs from group to group.

econml/ortho_forest.py Outdated Show resolved Hide resolved
econml/ortho_forest.py Show resolved Hide resolved
econml/ortho_forest.py Show resolved Hide resolved
econml/ortho_forest.py Outdated Show resolved Hide resolved
econml/ortho_forest.py Show resolved Hide resolved
econml/ortho_forest.py Outdated Show resolved Hide resolved
econml/ortho_forest.py Show resolved Hide resolved
econml/ortho_forest.py Show resolved Hide resolved
econml/tests/test_orf.py 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.

LGTM

@moprescu moprescu merged commit 70cd6d3 into master Feb 7, 2020
@moprescu moprescu deleted the moprescu/orf_inference branch March 27, 2020 15:26
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