Corrected cross-fitting loop #42
Conversation
| sumscore3 = np.mean(t * (1 - ptmx) / (ptmx * (1 - ptx))) | ||
| sumscore4 = np.mean((1 - t) * ptmx / ((1 - ptmx) * ptx)) | ||
| y1m1 = (t / ptx * (y - mu_t1_x)) / sumscore1 + mu_t1_x | ||
| y0m0 = ((1 - t) / (1 - ptx) * (y - mu_t0_x)) / sumscore2 + mu_t0_x | ||
| y1m0 = ( |
There was a problem hiding this comment.
Are there tests that check whether this snippet computes the right thing ?
Such computation blocks should better be isolated into small functions bzw.
There was a problem hiding this comment.
There aren't any test, I just made sure it gives similar performances with and without normalization on several dataset. I can declared that as an issue, #44
There was a problem hiding this comment.
If we want this lib to survive, we need to add a test suite in highest priority.
See #45
|
ok. So to recap
As for #43 only the code in get_estimation is tested, so the package is not properly equipped to check specifically that this PR does not break the code. Should we merge this as it is, and implement a more specific test suite? @bthirion @houssamzenati |
Previous cross-fitting loop was using an array of numpy array, and we suspected it to slow down the computation time since
multiply_robust_efficientis faster thanmed_dml. This commit optimizes the loop.In the end it appears the loop wasn't slowing down anything (
med_dmlis slower because it calls forest instances more often thanmultiply_robust).Also the new loop seems to give slightly more accurate estimation.