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

Add RiskyContrib Remark #123

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Add RiskyContrib Remark #123

merged 1 commit into from
Jul 20, 2021

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jun 18, 2021

This PR adds the remark associated with the new portfolio model in econ-ark/HARK#832.

The REMARK's repo is https://github.com/Mv77/RiskyContrib

@Mv77
Copy link
Contributor Author

Mv77 commented Jun 18, 2021

@llorracc asked me to use this process to see how clear and easy it is to post one's REMARK, so I'll give some impressions in following comments.

@Mv77
Copy link
Contributor Author

Mv77 commented Jun 18, 2021

A first is that the information on how to add the REMARK should really be in the README I think.

I remember I read somewhere that to add the REMARK I'd need to make a PR adding the markdown file with its identifying information, but I can not find that file now. It should be as easy to find as possible.

@Mv77
Copy link
Contributor Author

Mv77 commented Jun 18, 2021

It would also help to have some explanation of what some fields in the markdowns are. Most are self-explanatory, but I do not know the difference between e.g. "message" and "abstract".

@llorracc
Copy link
Collaborator

@MridulS this looks ready to me. If you can look at it before tomorrow's Econ-ARK call, then maybe we can merge it then? Otherwise we should construct a to-do list of whatever further steps would be required for it to be ready to merge.

PS. One thing that would be good to take care of is to respond to @Mv77's small comments on improving the REMARK submission documentation, including clarifying some of the metadata issues.

@Mv77
Copy link
Contributor Author

Mv77 commented Jun 23, 2021

Oh, I just thought of a additional thing.

It is clear that there should be a reproduce.sh file. It is not clear what that file should do regarding dependencies. For instance:

It is not clear who or what (a docker-setup script?, binder? the user?) will run reproduce.sh and based on that what should be done about requirements.

Because of that, my current handling of requirements in reproduce.sh might not be what you expect. Could someone verify that it works for the intended purpose and let me know if it does?

@llorracc
Copy link
Collaborator

llorracc commented Jun 25, 2021 via email

@llorracc
Copy link
Collaborator

@MridulS,

I want to merge this as soon as we can give @Mv77 an answer to his question about requirements.

Does my proposal above work?

@MridulS
Copy link
Member

MridulS commented Jul 15, 2021

@Mv77 The initial embodiment of the reproduce.sh file was that it runs inside the docker container with nbreproduce so whatever is installed with the reproduce.sh file is installed inside the docker container started by nbreproduce.
This has been conflated around different REMARKs right now (mostly due to poor documentation by me).

And yes @llorracc the idea to check if the environment is setup properly in reproduce.sh and then return the appropriate response is great but maintaining a bash script which works across Windows, unix wouldn't be that straight forward.

@Mv77 have you toyed around with nbreproduce yet? If you have the time could you try following the instructions available at https://github.com/econ-ark/KrusellSmith#krusellsmith ? The only thing that seems to be missing from https://github.com/Mv77/RiskyContrib at a first glance is some minimal documentation like https://github.com/econ-ark/KrusellSmith in the README about how to reproduce a local environment with either conda/pip or nbreproduce.
I can send in a PR later today to add those things to the README.

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 15, 2021

Thank you for the clarification!

@Mv77 have you toyed around with nbreproduce yet? If you have the time could you try following the instructions available at https://github.com/econ-ark/KrusellSmith#krusellsmith ? The only thing that seems to be missing from https://github.com/Mv77/RiskyContrib at a first glance is some minimal documentation like https://github.com/econ-ark/KrusellSmith in the README about how to reproduce a local environment with either conda/pip or nbreproduce.

I have not used nbreproduce yet. I'll toy with it and the KrusellSmith REMARK before today's meeting and report how it goes.

So my immediate goal is to make reproduce.sh work with nbreproduce in a way similar to the KrusellSmith REMARK. That helps a lot.

I can send in a PR later today to add those things to the README.

That would be very helpful, thank you.

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 15, 2021

The latest commits configure the repo so that environment.yml and reproduce.sh work with nbreproduce.

I have tested the repo using nbreproduce following the instructions in the KS remark and it works.

@MridulS, I've also updated the readme following KrusellSmith as a template. Please find it here: https://github.com/Mv77/RiskyContrib/blob/main/README.md

Are there further edits or tests that I should conduct?

@MridulS
Copy link
Member

MridulS commented Jul 16, 2021

@Mv77 I ran the REMARK locally with nbreproduce but the figures created don't match those in the Figures directory at https://github.com/Mv77/RiskyContrib/tree/main/Figures.
For example the LC_age_profiles pdf created by running in locally gives this:
LC_age_profiles.pdf

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 16, 2021

Interesting. Negative consumption usually means that the grid was not large enough. I will check what's going on.

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 16, 2021

I think I've fixed it. @MridulS could you re-run it and confirm?

The issue was an interaction of a quite unstable simulation that I'm running for demonstrative purposes and the recent RNG changes induced by Seb's "time-varying distribution" changes. I solved it by pinning the REMARK to a prior commit: the one when my new model was merged in.

In more detail, I am including a calibration with the extreme assumption that you can not touch your risky assets until you retire. Since you can not consume out of them for a while and they have high expected returns, they can grow quite large. Further, your permanent income might decrease and this combination can generate huge Risky Assets / Permanent Income, which is the state that I track. What I think happened is that Seb's RNG changes generated some sequence of specially high risky return draws that made Risky Assets / Permanent Income go off the grid in some of my simulations.

@MridulS
Copy link
Member

MridulS commented Jul 16, 2021

I was able to run this locally and reproduce the figures :)

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 19, 2021

Awesome, thank you for the review and help @MridulS.

What would be the next step? After it gets merged, does it automatically get added to the website?

@MridulS MridulS merged commit e8b2070 into econ-ark:master Jul 20, 2021
@MridulS
Copy link
Member

MridulS commented Jul 20, 2021

Lets try it :)

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.

3 participants