search

econ-ark / REMARK

Replications and Explorations Made using the ARK
https://econ-ark.github.io/REMARK/
Apache License 2.0
20 stars 56 forks source link

Add RiskyContrib Remark #123

Closed Mv77 closed 3 years ago

Mv77 commented 3 years ago

This PR adds the remark associated with the new portfolio model in https://github.com/econ-ark/HARK/pull/832.

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

Mv77 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

This is a good question.

Problem with creating a virtual environment is that it can take a long time, and would be confusing to someone not familiar with how they work. Seems to me that the best answer would be if reproduce.sh could test whether the requirements are satisfied, and if not should tell the user "you need to install further software requirements on your computer. From the command line where you can run reproduce.sh, please install the requirements with the command python -r " and then whatever the path would be. Or, whatever the right instructions are.

On Wed, Jun 23, 2021 at 4:05 PM Mateo Velásquez-Giraldo < @.***> wrote:

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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/econ-ark/REMARK/pull/123#issuecomment-867122095, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK73C4WJNHRXUMHUFS63TUI5ANANCNFSM464XIEPQ .

--

llorracc commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

Mv77 commented 3 years ago

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 commented 3 years ago

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

Mv77 commented 3 years ago

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 commented 3 years ago

Lets try it :)