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

Vasilis/randomstate #325

Merged
merged 7 commits into from
Nov 20, 2020
Merged

Vasilis/randomstate #325

merged 7 commits into from
Nov 20, 2020

Conversation

vsyrgkanis
Copy link
Collaborator

@vsyrgkanis vsyrgkanis commented Nov 19, 2020 •
edited
Loading

  • Fixed random state to be stateless and enable refitting the same instance with no change in all current _OrthoLearner classes. (fixes Results differ at each run #323)
  • Fixed some bugs in orthoiv related to passing W and to passing sample_weights to score in IntentToTreatDRIV.
  • Fixed some bugs related to random_state in orthoiv.
  • Fixed some bugs related to scoring in DRIV
  • Fixed some bugs related to treating sample_weight in DRIV

@vasilismsr
fixed random state in dml and ortho learner
a40cc7b
@vasilismsr
fixed random state to be stateless and enable refitting the same inst…
fd98c05 
…ance with no change. Fixed some bugs in orthoiv related to passing W and to passing sample_weights to score in IntentToTreatDRIV. Fixed some bugs related to random_state in orthoiv.
@vsyrgkanis vsyrgkanis added the bug Something isn't working label Nov 19, 2020
kbattocchi
kbattocchi reviewed Nov 19, 2020
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. While not strictly necessary, it might be nice to rewrite history so that this consists of two separate commits; one for the improved random state behavior and another for the bugfixes.

econml/_ortho_learner.py Outdated Show resolved Hide resolved
@kbattocchi
Copy link
Collaborator

Also, as long as you're fixing places where sample_weight is ignored, do you mind taking a look at the TODO in _BaseDRIVFinalModel.fit and seeing what can be done? I assume in the else branch we could just pass it (with **filter_none_kwargs, in case the model doesn't support it). But in the other branch should we be multiplying by the covariance-based weights, or just ignoring the sample weights, or what?

@vsyrgkanis vsyrgkanis merged commit e1abf19 into master Nov 20, 2020
@vsyrgkanis vsyrgkanis deleted the vasilis/randomstate branch November 20, 2020 10:51
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

@moprescu moprescu Awaiting requested review from moprescu

@heimengqi heimengqi Awaiting requested review from heimengqi

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Results differ at each run
3 participants
@vsyrgkanis @kbattocchi @vasilismsr