-
Notifications
You must be signed in to change notification settings - Fork 939
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
Replace all occurrences of get Pandas' get_dummies() with skLearn OneHotEncoder #1135
Replace all occurrences of get Pandas' get_dummies() with skLearn OneHotEncoder #1135
Conversation
…es of Pandas' get_dummies with skLearn's OneHotEncoder. Encoder lifespan: Reuses encoders for new estimate_effect() calls, and replaces existing encoders on CausalEstimator.fit(). Additional uses of get_dummies without side-effects or consistent encoding issues in do-Sampler Propensity Scores utilities also replaced for consistency. Signed-off-by: DAVID RAWLINSON <dave@causalwizard.app>
hi @amit-sharma are you able to take a look at this one? Thanks! |
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.
Thanks for your patience @drawlinson . The PR looks good. I have just one comment that I added.
Also, can you add at least a few tests showing that the encoder remains the same if a user calls estimate_effect twice without calling fit? This is a fairly large PR so it will be good to have a tests for at least a few of the estimators that are changed.
…bug in arg order for RegressionEstimator._do(). Signed-off-by: DAVID RAWLINSON <dave@causalwizard.app>
@amit-sharma I added some tests which aim to verify that encoding is consistent despite permuting data row order. It was a bity tricky working within the interfaces of the Estimator classes - I focused on estimate_effect() and do(x). With Regression estimators the effects of common causes are additive, so the ATE is almost unchanged despite changes in these variables! To check for consistency of these variables' encoding using I used the do() operator, the result of which is affected by common causes. In the process I discovered that the RegressionEstimator implementation of do() has a seemingly long-standing bug where the order of the arguments is reversed: CausalEstimator base class (treatment_value, dataframe): RegressionEstimator (dataframe, treatment_value): I've fixed RegressionEstimator to match the base class interface. I searched for all instances of Changed from: to: I'm sorry this has turned into a big PR but hopefully it's worth it! |
Build docs appears to be failing due to lack of disk space on the worker environment |
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.
Thank you so much, @drawlinson. The PR looks great now, merging.
Also, great that you were able to fix the bug. Much appreciated!
An earlier issue #1111 observed inconsistent behaviour from RegressionEstimator subclasses when new data for do() method had different rows than the originally fitted data, which caused categorical variables to be encoded inconsistently. This is because the do() operator allows unseen data to be processed with an existing Estimator.
This issue occurs because categorical encoding was using Pandas' get_dummies(), which does not allow additional data to be encoded using an existing encoder. An alternative, skLearn OneHotEncoder, returns an Encoder object which can be used to encode additional data consistently. skLearn is already a DoWhy dependency. For this reason skLearn is preferred over get_dummies.
This additional change goes further to replace all occurrences of get_dummies with OneHotEncoder, so that if functionality to process additional data is added to other classes in future (e.g. via do operator), the consistency bug won't happen again.
After the swap, all these changes are also heavily covered by existing tests, each time an Estimator is created and fitted, or when an effect is estimated.