Vasilis/local linear #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
vasilismsr
commented
Mar 16, 2019
•
- Optimized computations in causal tree to reduce overall ortho forest computation by two orders of magnitude. Improvement in fitting time primarily came from replacing internal loop over candidate splits with numpy array calculations. Improvement in predict time came from removing parallelism, which was adding data copy overhead.
- Added local linear correction (as in Athey, Friedman, Wager) to both ortho forest classes for better local fits.
- Minor edits in the ortho forest and causal tree architecture and default parameters.
…emoved the second order parameters from the moment and gradient function so that node splitting only depends on the first order intercepts and not on the second order linear part
…parate moment and mean gradient for first and second stage. Changed local linear ortho forest so that sample splitting happens in the same way as continuous ortho forest (i.e. just based on intercepts), while final only final stage estimation has the local linear part.
…ss. Small changes to discrete treatment forest defaults
…nterval coverage.
…/EconML into vasilis/local-linear
…t classes. threading seems to be avoiding the data copying overhead.
… causal_tree class.
…ully and was raising error in MacOS
kbattocchi
requested changes
Mar 20, 2019
Co-Authored-By: vasilismsr <[email protected]>
Co-Authored-By: vasilismsr <[email protected]>
moprescu
previously requested changes
Apr 3, 2019
econml/ortho_forest.py
Outdated
| T_hat = _cross_fit(model_T, X_tilde, T, split_indices, sample_weight=sample_weight) | ||
| Y_hat = _cross_fit(model_Y, X_tilde, Y, split_indices, sample_weight=sample_weight) | ||
| else: | ||
| T_hat = model_T.fit(X_tilde, T, sample_weight=sample_weight).predict(X_tilde) |
There was a problem hiding this comment.
The weights are never used in the first stage. I think we should omit passing them in here so the user doesn't have to pass in a weighted model for model_T and model_Y. This will potentially avoid some issues in the future. Same for the DiscreteTreatmentOrthoForest nuisance estimator.
There was a problem hiding this comment.
check out if I did it correctly
There was a problem hiding this comment.
@moprescu Looks like I need to dismiss this review to enable checkin - I assume that's okay with you.
kbattocchi
approved these changes
Apr 5, 2019
Vasilis has addressed the requested changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.