-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
minimal fix to resolve #707 #720
Conversation
causalml/metrics/visualize.py
Outdated
(outcome_col in df.columns and df[[outcome_col]].notnull().all().bool()) | ||
and (treatment_col in df.columns and df[[treatment_col]].notnull().all().bool()) | ||
or ( | ||
treatment_effect_col in df.columns | ||
and df[[treatment_effect_col]].notnull().all().bool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of all()
is already bool
. It doesn't need bool()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding the skipna=False
as an input argument so users can explicitly choose to allow NaNs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Is the idea for skipna to allow NaN's in the dataframe? So:
|
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
, andget_tmleqini
that required columns in the dataframes do not contain null values. An example test forget_cumlift
is included.Types of changes
What types of changes does your code introduce to CausalML?
Put an
x
in the boxes that applyChecklist
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