Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed column input names bugs. #398

Merged
merged 20 commits into from
Feb 19, 2021
Merged

Fixed column input names bugs. #398

merged 20 commits into from
Feb 19, 2021

Conversation

moprescu
Copy link
Contributor

@moprescu moprescu commented Feb 3, 2021 •
edited by kbattocchi
Loading

kbattocchi
kbattocchi requested changes Feb 3, 2021
View reviewed changes
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good; just a few questions

econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/utilities.py Outdated Show resolved Hide resolved
@moprescu moprescu force-pushed the moprescu/fix_metadata branch 2 times, most recently from 4bf0edc to 3e9d737 Compare February 5, 2021 19:06
kbattocchi
kbattocchi reviewed Feb 5, 2021
View reviewed changes
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments on the latest batch of changes.

econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Show resolved Hide resolved
econml/inference/_bootstrap.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Show resolved Hide resolved
econml/inference/_inference.py Outdated Show resolved Hide resolved
econml/inference/_inference.py Outdated Show resolved Hide resolved
econml/inference/_inference.py Outdated Show resolved Hide resolved
econml/inference/_inference.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
econml/utilities.py Outdated Show resolved Hide resolved
@moprescu moprescu force-pushed the moprescu/fix_metadata branch from 3e9d737 to 0194c4f Compare February 5, 2021 21:06
vsyrgkanis
vsyrgkanis previously requested changes Feb 6, 2021
View reviewed changes
econml/_cate_estimator.py Show resolved Hide resolved
econml/_cate_estimator.py Show resolved Hide resolved
econml/_cate_estimator.py Show resolved Hide resolved
econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
@moprescu moprescu force-pushed the moprescu/fix_metadata branch 2 times, most recently from d64fe28 to 5de06bd Compare February 12, 2021 15:45
Miruna Oprescu and others added 14 commits February 17, 2021 11:24
Fixed column input names bugs.
b4744fe 
* Fixed setting column names during refit and for categorical treatments
* Added tests
Added mode fail-safes to the get_input_columns utility function.
e21703d
Addressed PR comments. Fixed inference examples where d_t=1 is hardco…
46c8fd8 
…ded.
@kbattocchi
Update econml/_cate_estimator.py
56fd4c0 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/inference/_inference.py
1b61490 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/inference/_inference.py
2e24c04 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/inference/_inference.py
2ac2e3d 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/inference/_inference.py
86c2180 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/orf/_ortho_forest.py
abe64f0 
Co-authored-by: Keith Battocchi <[email protected]>
@kbattocchi
Update econml/utilities.py
2b38133 
Co-authored-by: Keith Battocchi <[email protected]>
Addressed PR comments and fixed notebooks.
2c64510
Changed d_t=1 for effect_inference and coef__inference to d_t=None an…
e5d6b49 
…d propagated changes
Changed calls to private input names to public estimator calls
d7d49c8
Refactor discrete treatment logic and remove old patterns.
173460d
@moprescu moprescu force-pushed the moprescu/fix_metadata branch from 54afd0f to 173460d Compare February 17, 2021 17:09
kbattocchi
kbattocchi requested changes Feb 18, 2021
View reviewed changes
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. However, there are a few issues with how categories are treated that need to be addressed.

econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/_cate_estimator.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/metalearners/_metalearners.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
econml/orf/_ortho_forest.py Outdated Show resolved Hide resolved
@moprescu moprescu force-pushed the moprescu/fix_metadata branch from 25f4a57 to 42c7d6e Compare February 18, 2021 19:26
kbattocchi
kbattocchi approved these changes Feb 19, 2021
View reviewed changes
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kbattocchi kbattocchi dismissed vsyrgkanis’s stale review February 19, 2021 17:51

I believe Miruna has addressed your concerns, and this review is blocking the merge.

@kbattocchi kbattocchi merged commit a4ba61c into master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbattocchi kbattocchi kbattocchi approved these changes

@heimengqi heimengqi Awaiting requested review from heimengqi

@vsyrgkanis vsyrgkanis Awaiting requested review from vsyrgkanis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug in default variables when refitting Bug in default treatment name parsing from pandas Dataframe
3 participants
@moprescu @kbattocchi @vsyrgkanis