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.

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

Implement DynamicDML #446

Merged
merged 36 commits into from
Aug 11, 2021
Merged

Implement DynamicDML #446

merged 36 commits into from
Aug 11, 2021

Conversation

moprescu
Copy link
Contributor

  • DynamicDML with all the OrthoLearner functionality
  • API tests similar to the DML ones

* DynamicDML with all the OrthoLearner functionality
* API tests similar to the DML ones
@moprescu moprescu force-pushed the moprescu/dynamicdml branch from 16dd7b7 to 762e0e4 Compare March 31, 2021 19:25
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.

Here are some comments - please address what you're able to.

econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/tests/dgp.py Show resolved Hide resolved
@moprescu moprescu force-pushed the moprescu/dynamicdml branch from e9bffca to 5f6da40 Compare April 10, 2021 12:02
@moprescu moprescu marked this pull request as ready for review August 3, 2021 00:18
@moprescu moprescu force-pushed the moprescu/dynamicdml branch from 2243b26 to 4636257 Compare August 3, 2021 17:08
@vsyrgkanis
Copy link
Collaborator

Could you also add a snippet of code in the README, as it seems a substantive addition to advertise!

Copy link
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.

The text that lies under the table in the summary results is good for a static setting, but might not be the best for the summary results displayed in the dynamic setting. So we might want to not show that text here in the summary

@vsyrgkanis
Copy link
Collaborator

In the plot of effects in the notebook, is theta_0 the effect from treatments in period 0 or the effect of treatment at lag 0 from the outcome. Similarly for the rest of the params.

@vsyrgkanis
Copy link
Collaborator

In the notebook when you say: "number of panels" do you mean "number of units"

@vsyrgkanis
Copy link
Collaborator

The results in the notebook, even with 5000 units are very poor! We need to investigate this before we push:
image

@vsyrgkanis
Copy link
Collaborator

Results are almost identical when n_periods =1
DynamicDML
image
LinearDML
image

So there is something when we have multiple periods that gets messed up

@vsyrgkanis
Copy link
Collaborator

Oh!! It was just the parameters of lassocv you set in the notebook that mess thigns up. So just leae the default tol and the default alpha grid. That improves performance significantly
image

and also matches the LinearDML.

Also I would say we might want to reverse the parameters to mean what you are descrbing and not the lag version that was in the prototype.

So the (T0)_0 should mean the effect of period 0 on final outcome and (T0)_m the effect of final period.

Currrently it's the reverse and that messes also the "effect(X, T0, T1)"
here one needs to give T0 and T1 in the reverse order too.

@moprescu
Copy link
Contributor Author

moprescu commented Aug 6, 2021

Oh!! It was just the parameters of lassocv you set in the notebook that mess thigns up. So just leae the default tol and the default alpha grid. That improves performance significantly
image

and also matches the LinearDML.

Also I would say we might want to reverse the parameters to mean what you are descrbing and not the lag version that was in the prototype.

So the (T0)_0 should mean the effect of period 0 on final outcome and (T0)_m the effect of final period.

Currrently it's the reverse and that messes also the "effect(X, T0, T1)"
here one needs to give T0 and T1 in the reverse order too.

Fixed!

@moprescu moprescu closed this Aug 6, 2021
@moprescu moprescu reopened this Aug 6, 2021
econml/tests/dgp.py Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
econml/dml/dynamic_dml.py Outdated Show resolved Hide resolved
@heimengqi heimengqi enabled auto-merge (squash) August 11, 2021 17:13
@heimengqi heimengqi merged commit 156d2df into master Aug 11, 2021
@heimengqi heimengqi deleted the moprescu/dynamicdml branch August 11, 2021 22:46
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.

4 participants