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

[WIP] Find value function crossings in dcegm.calcMultilineEnvelope #758

Merged
merged 9 commits into from
Jul 21, 2020

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jul 11, 2020

This PR augments dcegm.calcMultilineEnvelope by finding the exact coordinates of the kinks generated in the value function when computing its upper envelope. This lets us find the exact position of secondary kink points in DCEGM.

There are a couple of coding decisions on how to use these points that I would like to consult:

  1. Should these points be incorporated to the m-grid inside the calcMultilineEnvelope function or returned for other methods to do that and also use them for other purposes (e.g. plotting)?. My guess is do both.

  2. What is your preference on how to handle the discontinuity of interpolating functions at these points? My idea is: if the crossing happens at some m, then add (m) and ( m+ np.finfo(float).eps) to the grid, with the corresponding values of c and v. Then generate the interpolating functions. Is there an approach that you would prefer?

@llorracc
Copy link
Collaborator

@Mv77 could you make this an option rather than an unavoidable part of the code, and make a PR for a modification to the DemARK which illustrates the point. (The issue is basically whether the time cost in finding those points is worth the increased accuracy; it would be nice to do some timing tests at some point, but those are not needed now).

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 16, 2020

The last commit makes computing the crossing points optional through an added boolean argument findXings. If findXings == True, the crossing points are computed and added to the gridpoints, and their m coordinate returned.

This feature is illustrated in econ-ark/DemARK#126 . That PR won't work until this one is merged, but you can see the results in my version of the notebook here: https://github.com/Mv77/DemARK/blob/upperEnvelopeExample/notebooks/DCEGM-Upper-Envelope.ipynb (at the very end).

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 16, 2020

While checking for backwards compatibility of this PR, I noticed the endogenous-retirement REMARK is now broken. It seems to have to do with the change in how distributions are handled.

However, the dcegm test passed.

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 16, 2020

While checking for backwards compatibility of this PR, I noticed the endogenous-retirement REMARK is now broken. It seems to have to do with the change in how distributions are handled.

The distributions were the trouble indeed. Fixed in econ-ark/REMARK#70

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 17, 2020

The last commit adds a function for computing primary kinks. This is completely optional and done outside of calcLogSumChoiceProbs, taking its result as input.

@llorracc llorracc merged commit 028f34c into econ-ark:master Jul 21, 2020
@Mv77 Mv77 deleted the evelope-xings branch December 17, 2020 16:33
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.

2 participants