Meta Learner allow N/A

[xhulianoThe1]

Fixing sklearn utils function that checks array so it has the ability to allow NA values when doing effect estimation.
See issue #827

Signed-off-by: Xhuliano Brace [email protected]

[fverac]

Thanks for the PR! For completeness, would you also be able to make changes that would allow missing values during effect estimation for other ests that support missing values in X, like DRLearner, DML, and NonParamDML as well?

[xhulianoThe1]

Yes! Will update PR.

[fverac]

Hey, I never followed up on this.

Seems your latest commit broke some tests. I haven't quite looked at the errors in the tests, but speaking to the commit itself, I would generally recommend against adding the additional check_inputs lines you've added, because these checks already exist somewhere in the control flow (you can look at the errors/traceback generated in the colab notebook linked in #827).

So in order to allow missing values during effect estimation for DRLearner, DML, and NonParamDML, we'd have to address the already-existing missing value checks in the control flow (via expand_treatments, for instance).

Another thing I'm noticing is that while your metalearner-specific fixes may have allowed for calling const_marginal_effect with missing values in X, I still don't think it would work when calling effect (or marginal effect, const_marginal_ate, etc.) with missing values in X (because of the checks that expand_treatments does, as can be referred in the colab notebook linked)

So it seems the next step is to tackle how we might allow expand_treatments to allow missing values in X.

Finally, another thing we'd want to do before we merge is create corresponding tests to for the functionality we've added (via adding to econml/tests/test_missing_values.py). So tests that actually double check that we can call XLearner(...).fit(..).effect(X) when X has missing values.

I know I just shared a lot of comments, and I don't know how much bandwidth/interest you have in bringing this PR to completion, so feel free to let me know if you'd rather the EconML team try to wrap the PR up by picking up where you left off (though I can't say when we'd get to it).

[xhulianoThe1]

Hey, I never followed up on this.

Seems your latest commit broke some tests. I haven't quite looked at the errors in the tests, but speaking to the commit itself, I would generally recommend against adding the additional check_inputs lines you've added, because these checks already exist somewhere in the control flow (you can look at the errors/traceback generated in the colab notebook linked in #827).

So in order to allow missing values during effect estimation for DRLearner, DML, and NonParamDML, we'd have to address the already-existing missing value checks in the control flow (via expand_treatments, for instance).

Another thing I'm noticing is that while your metalearner-specific fixes may have allowed for calling const_marginal_effect with missing values in X, I still don't think it would work when calling effect (or marginal effect, const_marginal_ate, etc.) with missing values in X (because of the checks that expand_treatments does, as can be referred in the colab notebook linked)

So it seems the next step is to tackle how we might allow expand_treatments to allow missing values in X.

Finally, another thing we'd want to do before we merge is create corresponding tests to for the functionality we've added (via adding to econml/tests/test_missing_values.py). So tests that actually double check that we can call XLearner(...).fit(..).effect(X) when X has missing values.

I know I just shared a lot of comments, and I don't know how much bandwidth/interest you have in bringing this PR to completion, so feel free to let me know if you'd rather the EconML team try to wrap the PR up by picking up where you left off (though I can't say when we'd get to it).

Hey, sorry I have had very limited bandwidth and need to deep dive in the intricacies of the code base some more. I will find try and find time to plug the holes based on what you commented and reupdate the draft as I go along. Then if I can't finish in a timely fashion whenever one of the members has bandwidth they can feel free to take it on as to not block this output. Will try to find time this next week or two and reopen a clean request!! @fverac