-
Notifications
You must be signed in to change notification settings - Fork 721
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
Add causal analysis solution #447
Conversation
7471971
to
9043b97
Compare
9043b97
to
32d321e
Compare
32d321e
to
da0ff45
Compare
da0ff45
to
98b09af
Compare
|
||
return insights, result | ||
|
||
if self.feature_names is None: |
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.
Is this logic maybe in some sklearn utility
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.
If we are in a warm start why are we not using the same feature names as in the first time.
We could be getting some weirdness here if user passes once a numpy array and then a pandas data frame with warm start
98b09af
to
9dc967b
Compare
categorical: array-like of int, str, or bool | ||
The features which are categorical in nature, expressed as either column indices, | ||
column names, or boolean flags indicating which columns to pick | ||
heterogeneity_inds: array-like of int, str, or bool, or None or list of array-like elements or None, default None |
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.
To avoid confusion I think we should make the default of heterogneity inds be 'all', which means use all variables, and None or empty list, should mean use no variables for heterogeneity.
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.
if it's an empty list, forest option will not work? it requires X? and also all the local view will not work neither
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.
I have made sure that empty h_inds now work with linear final models (even with local effects, although there's no point since there are no features to cause variation so all samples have the same results); I have not changed the spec of the method (e.g. enabling 'all'
) because that seems too risky at this stage for this release. As Maggie notes, empty h_inds will not work with forest dml and I don't see any easy path to making it work.
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.
Sounds good. Let's just elaborate a bit more in the docstring that to use no features for heterogneiety use the empty list.
feature_names: list of str, default None | ||
The names for all of the features in the data. Not necessary if the input will be a dataframe. | ||
If None and the input is a plain numpy array, generated feature names will be ['X1', 'X2', ...]. | ||
upper_bound_on_cat_expansion: int, default 5 |
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.
this arg seems not being applied to any place?
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.
Yeah, there's a TODO later noting that it's not yet implemented but should be.
I might even do the same uniformization even for local effects and maintain the feature_index inner index with the single feature name that we run the analysis for. |
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.
LGTM
c6f5f1e
to
878b54d
Compare
878b54d
to
6d4cb76
Compare
No description provided.