uber / causalml

Uplift modeling and causal inference with machine learning algorithms
Other
4.87k stars 756 forks source link

minimal fix to resolve #707 #720

Closed ras44 closed 7 months ago

ras44 commented 7 months ago

Proposed changes

This is a minimal fix to resolve the issue described in #707. The fix contains assertions in the functions get_cumlift, get_qini, get_tmlegain, and get_tmleqini that required columns in the dataframes do not contain null values. An example test for get_cumlift is included.

Types of changes

What types of changes does your code introduce to CausalML? Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

Further comments

None

ras44 commented 7 months ago

Let's merge this PR as is and open a new PR for adding an option for skipna=False. Thanks for the contribution.

Took a quick look at this and not sure how the skipna=False argument should work:

https://github.com/uber/causalml/blob/d26e0891f3aa6279e359fe6abe1dd77e1f125c8a/causalml/metrics/visualize.py#L115C5-L115C5

Is the idea for skipna to allow NaN's in the dataframe? So: