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

DCEGM rework #1100

Merged
merged 22 commits into from
Jan 12, 2022
Merged

DCEGM rework #1100

merged 22 commits into from
Jan 12, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jan 9, 2022

This PR will contain work addressing #1062.

The idea of my proposed rework will be to provide general-purpose tools to speed-up the work of someone implementing DCEGM. The current tools seem to intend to apply DCEGM as presented in the original paper.

Thus, for instance, I want to replace the current tool which

  • Takes the upper envelope of the value and consumption function of a liquidity constrained agent with CRRA utility who is choosing his consumption.
    With a tool that
  • Takes a list of (x,y) segments and returns their upper envelope.

It is my view that optimizations and specific uses of the general purpose tools are part of the specific models and should be done in their files, when they are implemented.

The version in the PR also uses numba.jit and Pablo's interpolation library to speed up things.

@Mv77
Copy link
Contributor Author

Mv77 commented Jan 9, 2022

The current version has the rework of the main method.

I still need to

  • Add proper docstrings.
  • Remove old methods that are no longer needed.
  • Check if there are additional methods worth jitting.
  • Update the tests and DemARKs.
  • Edit the changelog.

@Mv77
Copy link
Contributor Author

Mv77 commented Jan 9, 2022

@alanlujan91, @llorracc tells me that you might be interested in this rework. I think it is mostly done and just needs to be documented, I can do that relatively soon.

We can then chat about G2EGM, which Chris said you might want to look into. I had a go at debugging the current version long, long ago. That partial and old work is here: #782.

I think the nature of the bugs that still remain might have to do with the same issues I see with the current DCEGM work. If I recall correctly, there were various methods that mixed numerical tasks with model-specific details. Separating those two things might be a worthwhile endeavor if one wants to get it to work. It's what I'd do at least. Nevertheless, the numerical procedures involved in G2EGM seem much more complex.

@alanlujan91
Copy link
Member

This is great! Let me know when you would like me to review it.

@alanlujan91 alanlujan91 self-requested a review January 10, 2022 19:21
@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #1100 (a6b6cd3) into master (9562caf) will increase coverage by 0.08%.
The diff coverage is 52.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1100      +/-   ##
==========================================
+ Coverage   73.58%   73.66%   +0.08%     
==========================================
  Files          69       69              
  Lines       10592    10560      -32     
==========================================
- Hits         7794     7779      -15     
+ Misses       2798     2781      -17     
Impacted Files Coverage Δ
HARK/dcegm.py 43.18% <43.52%> (-6.45%) ⬇️
HARK/tests/test_dcegm.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9562caf...a6b6cd3. Read the comment docs.

@Mv77 Mv77 changed the title [WIP] DCEGM rework DCEGM rework Jan 11, 2022
@Mv77
Copy link
Contributor Author

Mv77 commented Jan 11, 2022

This is ready for review. The updated DemARK is in econ-ark/DemARK#181

@llorracc
Copy link
Collaborator

@alanlujan91, you are the natural reviewer, given your earlier conversations. I'm not sure whether you can merge it if you approve of it, but if not just post a comment that you think it is ready to merge and email me and I'll merge.

Copy link
Member

@alanlujan91 alanlujan91 left a comment

Choose a reason for hiding this comment

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

This PR is in great shape!

Given that we are using Econ-Forge's interpolation I have a couple of questions about whether we could numba-fy all of the tools here. It could allow for some tasks to be done in parallel simply changing a few ranges to pranges which could make some processes faster, although there could be drawbacks when applied to small loops.

HARK/dcegm.py Outdated Show resolved Hide resolved
HARK/dcegm.py Outdated Show resolved Hide resolved
HARK/dcegm.py Show resolved Hide resolved
HARK/dcegm.py Show resolved Hide resolved
HARK/dcegm.py Outdated Show resolved Hide resolved
HARK/dcegm.py Show resolved Hide resolved
@alanlujan91 alanlujan91 merged commit f338c97 into econ-ark:master Jan 12, 2022
@Mv77 Mv77 deleted the plumbing/dcegm branch January 24, 2022 15:13
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
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.

5 participants