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

[RFC] Include DCEGM main function in HARK #226

Merged
merged 8 commits into from
Apr 11, 2019
Merged

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Feb 19, 2019

To keep the various figurative balls rolling, it's convenient to start including some the tools in a tool-y manner. The upper envelope step from DCEGM is such a tool, and it is important for the DCEGM REMARKs, the NEGM DEMARK and REMARK, and the TFI deliverable.

@mnwhite I've taken out the function and thrown it in a file. I hope this makes it easier for you to look through.

Compared to the DCEGM PR it is cleaned up a bit, which should make it a) easier to read, b) slightly faster, and c) slightly more correct (an index was off by one).

The reason why I would love to have this merged as a function and not bundled with some discrete/continuous type class, is that it's probably (or certainly) going to be relevant beyond what we can achieve with inheritance. By that I mean, that some users will want to call this directly, not just through some class we might define. One example of this would be the REMARKs and DEMARKs mentioned above.

To be discussed

  • Names
  • Exact location (what file,...)
  • Good tests? (slightly optional, as momentum is first priority here)
  • (is lower_extrap really needed, I think the code above should take care of this not happening)
    Any any other comments you might have.

P.S. I'm returning a copy of the common grid. This may seem weird, but in the original paper they discuss adding the exact crossing points to the grid for increased precision. I may come back to this, but it didn't seem super important. It can make the discrete choices a bit more precisely located I guess. It's 1D though, so we can afford quite a lot of points (though the NEGM context might be an exception).

Release notes: see here

@pkofod
Copy link
Contributor Author

pkofod commented Feb 19, 2019

For those interested, this again shows how relatively straight forward it is to add tests for the functions we provide. The test I added takes a sequence of points that "could" come from an necessary (but not sufficient) EGM step. It's designed to fold back over itself once, and what I'm basically checking is that the "upper envelope" is actually equal to the value of the segments that is upper... More testing would include verifying that upperM[0], upperC[0], and upperV_T[0] are indeed 0.0, such that stuff like lower_extrap can be avoided.

@pkofod
Copy link
Contributor Author

pkofod commented Feb 27, 2019

I didn't mention it explicitly here, though it's visible from the references above, but econ-ark/DemARK#27 shows this in action.

@pkofod
Copy link
Contributor Author

pkofod commented Mar 18, 2019

@mnwhite it'd be quite useful if you could voice you opinion on where you think these things should reside. Right now, these functions are used in a DemARK econ-ark/DemARK#27 and REMARK econ-ark/REMARK#9 that are getting close to ready, but they're both "copying code" from this PR to be able to do so, so it'd be great if they could just be from HARK.xxx import yyy, zzz statements instead.

I'm curious about your thoughts on naming, placement in the folder tree, and if it's fine that they're introduced as separate functions for now, with inclusion in some generalized ConsIndShkConsumer in the future when we discuss the organization/structure-update.

@mnwhite
Copy link
Contributor

mnwhite commented Mar 18, 2019 via email

@pkofod
Copy link
Contributor Author

pkofod commented Mar 18, 2019

Tomorrow is entirely an Econ-ARK day, expect long email/post about this.

Sounds great! Whatever you prefer. Edit: if you have time in the morning we could also do a quick call.

@mnwhite
Copy link
Contributor

mnwhite commented Mar 21, 2019 via email

@pkofod
Copy link
Contributor Author

pkofod commented Apr 5, 2019

This would also be nice to discuss and merge before a new release cc @shaunagm @mnwhite

@pkofod
Copy link
Contributor Author

pkofod commented Apr 11, 2019

Okay, so I pushed some changes last night, and I think we should merge. The changes were: rename to dcegm and rename dcegmSegments to calcSegments. Tests were adjusted accordingly. The intended use is then

import HARK.dcegm

dcegm.calcSegments(...)
dcegm.calcMultilineEnvelope(...)

This should be sufficiently clear to users of these functions, which will probably not be many, once the discrete choice solver class lands. The latter calls the former, so the former is really only split out for a) convenience when debugging and b) DemARKs etc. We may want to add more convenience later, but I think this is what we need, and it should be merged soon (now?)

@llorracc
Copy link
Collaborator

llorracc commented Apr 11, 2019 via email

@llorracc llorracc merged commit dfa22bc into econ-ark:master Apr 11, 2019
@shaunagm
Copy link
Contributor

@pkofod @llorracc Does "Adds DCEGM main function" make sense for a release note? If not, please provide alternative, thanks.

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