Fix missing value when calculate cumulative lift

[jpansnap]

Proposed changes

Noticed that if there are missing values for the outcome column when calculate cumulative lift, the current .cumsum() function default as skipna=False which will return NaN when there are missing values as a results the calculation of lift could be off

Propose change to .fillna(0).cumsum(skipna=True).astype(float) (change to float to avoid the division by zero error as well)

Types of changes

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

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices 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.

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc. This PR template is adopted from appium.

[paullo0106]

LGTM

[ras44]

Just took a quick look at this: could the same be achieved using only the .fillna(0) element and skipping the skipna=True and .astype(float)?

[jpansnap]

Just took a quick look at this: could the same be achieved using only the .fillna(0) element and skipping the skipna=True and .astype(float)?

Just .fillna(0) will give the division by zero error for example the first record is missing - that's why used .asype(float)

Adding skipna=True just trying to be more explicit and also in case some weird cased that filling na didn't work

[vincewu51]

We had a discussion this morning, just want to provide some of our discussion here:

1. it seems to me fillna(0) should be a decision by the user. Should we consider to provide the option to the user what is the value they want to fill
2. If there is NA in the data, shouldn't the user handle it at the beginning of the model instead of at the plotting stage? Maybe we should just throw an error and suggest user to handle NA by themselves.
3. To the very least, we should let user know their data has NA and causalml filled it.

What do you think?

[jpansnap]

We had a discussion this morning, just want to provide some of our discussion here:

1. it seems to me fillna(0) should be a decision by the user. Should we consider to provide the option to the user what is the value they want to fill
2. If there is NA in the data, shouldn't the user handle it at the beginning of the model instead of at the plotting stage? Maybe we should just throw an error and suggest user to handle NA by themselves.
3. To the very least, we should let user know their data has NA and causalml filled it.

What do you think?

This is a quick fix as the main issue right now for this function which will just return empty values - not only plotting but also the AUUC will be off.

For 2. For my use case, I didn't use CausalML for modeling but just plotting - I would assume we want to provide support for individual module as well.
For 1 and 3 - we can modify the code to be more robust please let me know which way we'd prefer to.

[vincewu51]

We had a discussion this morning, just want to provide some of our discussion here:

1. it seems to me fillna(0) should be a decision by the user. Should we consider to provide the option to the user what is the value they want to fill
2. If there is NA in the data, shouldn't the user handle it at the beginning of the model instead of at the plotting stage? Maybe we should just throw an error and suggest user to handle NA by themselves.
3. To the very least, we should let user know their data has NA and causalml filled it.

What do you think?

This is a quick fix as the main issue right now for this function which will just return empty values - not only plotting but also the AUUC will be off.

For 2. For my use case, I didn't use CausalML for modeling but just plotting - I would assume we want to provide support for individual module as well. For 1 and 3 - we can modify the code to be more robust please let me know which way we'd prefer to.

Got it, how about we go with option 1(provide the option to the user)? It is more robust and gives users more flexibility.

[jeongyoonlee]

This is a quick fix as the main issue right now for this function which will just return empty values - not only plotting but also the AUUC will be off.

For 2. For my use case, I didn't use CausalML for modeling but just plotting - I would assume we want to provide support for individual module as well. For 1 and 3 - we can modify the code to be more robust please let me know which way we'd prefer to.

What about throwing an error with a message informing that there's NaN in the input instead of returning null? It's not a good practice to impute NaN silently. Another option is that we can add an input argument for users to specify whether to impute NaN or not with the default being no imputation.

[jpansnap]

This is a quick fix as the main issue right now for this function which will just return empty values - not only plotting but also the AUUC will be off.
For 2. For my use case, I didn't use CausalML for modeling but just plotting - I would assume we want to provide support for individual module as well. For 1 and 3 - we can modify the code to be more robust please let me know which way we'd prefer to.

What about throwing an error with a message informing that there's NaN in the input instead of returning null? It's not a good practice to impute NaN silently. Another option is that we can add an input argument for users to specify whether to impute NaN or not with the default being no imputation.

Yeah agree, we could throw the error and add the input argument. Also saw the comment in #720