Skip to content

DMLIV/DRIV#460

Merged
heimengqi merged 22 commits intomasterfrom
mehei/driv
Aug 12, 2021
Merged

DMLIV/DRIV#460
heimengqi merged 22 commits intomasterfrom
mehei/driv

Conversation

@heimengqi
Copy link
Copy Markdown
Contributor

No description provided.

@heimengqi heimengqi marked this pull request as ready for review June 21, 2021 20:02
@heimengqi heimengqi requested review from kbattocchi, moprescu and vsyrgkanis and removed request for kbattocchi and vsyrgkanis June 21, 2021 20:03
Copy link
Copy Markdown
Member

@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 mostly looks good; added a few suggestions and I'm still going through the DRIV changes and will add a second review once I'm done with that.

Comment thread econml/dr/_drlearner.py Outdated
Comment thread econml/dr/_drlearner.py Outdated
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/iv/dml/_dml.py
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/sklearn_extensions/linear_model.py Outdated
Comment thread econml/sklearn_extensions/linear_model.py
Comment thread econml/tests/test_dowhy.py
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/iv/dml/_dml.py Outdated
Comment thread econml/sklearn_extensions/linear_model.py Outdated
Comment thread econml/sklearn_extensions/linear_model.py Outdated
Comment thread econml/iv/dml/_dml.py
Comment thread econml/iv/dml/_dml.py Outdated
Copy link
Copy Markdown
Collaborator

@vsyrgkanis vsyrgkanis left a comment

Choose a reason for hiding this comment

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

DMLIV Looks good except for the comments I already raised.

Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
@vsyrgkanis
Copy link
Copy Markdown
Collaborator

I reviewed code up and before the IntentToTreat!
Looks great. Only one potential major change is requested. Have not run quick sanity checks also. Will do once I finish the IntentToTreat code pass.

Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py
@vsyrgkanis
Copy link
Copy Markdown
Collaborator

I finished going over the whole driv code. So all code seems good, except for comments raised!

Comment thread econml/iv/dr/_dr.py
Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py Outdated
Comment thread econml/iv/dr/_dr.py
Copy link
Copy Markdown
Collaborator

@vsyrgkanis vsyrgkanis left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread econml/iv/dr/_dr.py Outdated
@heimengqi heimengqi requested a review from kbattocchi August 9, 2021 16:51
Copy link
Copy Markdown
Member

@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.

Great work!

@heimengqi heimengqi enabled auto-merge (squash) August 11, 2021 17:33
@heimengqi heimengqi merged commit 063784e into master Aug 12, 2021
@heimengqi heimengqi deleted the mehei/driv branch August 12, 2021 21:40
This was referenced Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants