add estimate_counterfactual_outcomes for W in {0,1} #403

[ras44]

A start to providing the functionality requested in #403 (comment):

Finally, it would be nice to provide a short function which performs the above steps, or at least add this information to our algorithm reference.

This does not include the general calculation if multiple, or non {0,1} outcomes exist, but happy to consider that if helpful.

[halflearned]

Hi @ras44, thank you for the changes! The code looks fine to me, just have a few comments on style and documentation.

[ras44]

@halflearned Thanks for your review! I've tried to address all items, but please let me know if I missed anything.

I also condensed the format of the returned list slightly (use Y.hats[['0']] or Y.hats$`0` instead of the redundant Y.hats$Y.hat.0) and included a test that W.orig is binary until multi-level treatments are supported. Let me know if you'd prefer something different or to revert.

Two additional questions:

• would it make sense to separate estimate_counterfactual_outcomes() into a new file (estimate_counterfactual_outcomes.R) in r-package/grf/R?
• @jtibshirani mentions including documentation on the functionality in Obtain (conditional) outcome estimates and confidence intervals #403 (comment), would that be helpful?

[ras44]

@halflearned just pinging you on this PR review, thanks

[halflearned]

Hi @ras44, sorry for the delay. To answer your questions.

• Separating estimate_counterfactual_outcomes() into a new file
  I think it's fine the way it is right now, but I'll defer this one to @jtibshirani.

• Documentation on the functionality in Obtain (conditional) outcome estimates and confidence intervals #403
  Yes, it'd be super helpful to include the definitions of Y.hat.1 and Y.hat.0 as explained in that issue.

Also, could you add a short test to the R test codebase?

[ras44]

Hi @halflearned, thanks for your review. I hope the above commits address the outstanding issues, but please let me know if I can make any other modifications 👍

[halflearned]

Hey @ras44, thanks for all the work! Just letting you know that your PR needs some additional vetting from other members of the team, and we'll get back to you as soon as possible. Sorry for the wait! Smaller PRs like this usually don't take long, but you just happen to catch us in a bit of a busy couple of weeks.

[ras44]

no problem, @halflearned Thanks for your note! 👍

[ras44]

@halflearned @swager I just merged in master to keep this branch fresh, it should be ready to go for review whenever you're ready 👍

[erikcs]

Hi @ras44, can you please run Rscript build_package.R on your machine? (there are some error messages there)

[ras44]

@erikcs thanks for your note. Just to make sure I'm looking at the right error messages, did you mean the linting errors? eg:

tests/testthat/test_regression_forest.R:212:49: style: Put spaces around all infix operators.

If so, I see linting errors arise for many of the files that I merged in from master, not only the ones that I have worked on in my branch.

[erikcs]

Can you please try to remove.packages("lintr"); devtools::install_github("jimhester/lintr")? (you can get rid of all the Trailing whitespace is superfluous errors by opening your files in Atom and just saving them there, Atom removes unnecessary white space).

[ras44]

It looks like the jimhester/lintr resolved the issues I was seeing with other files. I then fixed the linting errors in the two files modified for this PR. Please let me know if you see any further issues.

[erikcs]

Thanks @ras44, sorry for the trouble (I should add a note on that (far from obvious) lintr incompatibility to the contributing documentation)

[ras44]

No problem 👍 Thanks for your help!

[ras44]

@swager just pulled in master to keep this PR current. It should be ready for review whenever you're ready, thanks!

[ras44]

@swager just following up- I'm happy to close this PR if it's not moving in the right direction 👍

[swager]

First of all, I'm sorry @ras44 for my very slow response on this. We spent quite a bit of time discussing this internally.

When adding new functions to GRF, there's always a trade-off between how much value they add, and the cost of maintaining them. In this case, estimating the counterfactual response surfaces is not one of the most common use cases of the package -- and, at the same time, there's very low overhead for a user who wants this functionality to write their own script (for example, the function est_counterfactual_outcomes doesn't call into any private functions, so someone who wanted to use it could just copy-paste the function and use it directly). For this reason, we're not going to merge this function now.

On a brighter note, I'll mention that @erikcs is currently working on an add-on package to GRF with a focus on policy learning that will also support similar functionality (i.e., provide estimates of treatment-specific regression surfaces based on causal_forest, including in the case with many treatments). So hopefully in the near future that'll also be an option for users who need functionality of this type.

[ras44]

@swager no problem- thanks for following up!