econ-ark / DemARK

Demonstrations of how to use material in the Econ-ARK
https://econ-ark.github.io/DemARK/
Apache License 2.0
32 stars 93 forks source link

Fisher two period #206

Open AMonninger opened 1 year ago

AMonninger commented 1 year ago

This PR adjusts the notebook in the following ways:

  1. Updating Seaborn import to avoid warning.
  2. Changing numbers of decimal places shown in widgets in line with step size.
  3. Writing all labels in Latex notation with lower values for time periods.
  4. Replace notation from "RHi" and "RLo" to "RInit" and "RNew".
  5. Figure 4 works now (Problem was that PermGroFac cannot be exactly zero).
AMonninger commented 1 year ago

@llorracc can you have a look at it?

llorracc commented 1 year ago

Updating Seaborn import to avoid warning.

I wanted you to try to fix things so that the warning was no longer presented, regardless of the log level.
That is, fix the bug, don't hide it. This presumably involves updating the version of seaborn that is installed in the HARK/binder/requirements.txt file; when you do that, you should trigger a test of whether that breaks any of the other DemARKs. If you are not sure how to do all of this, please ask Seb or Matt.

(Also, when I run your notebook, I am still getting the error message, so I'm puzzled you are not).

Figure 4 works now (Problem was that PermGroFac cannot be exactly zero).

Please post it as an issue in HARK that we should be able to accommodate PermGroFac of zero.

Writing all labels in Latex notation with lower values for time periods.

That was not the task. The task was to have identical variable names in the python code as in the plot labels. So the VARIABLE in python should not be B1, it should be b_1. (Please match the case in the lecture notes, where variables are lower case).

PS. I've turned this into a "draft" PR. Am trying to learn how draft PR's work.

mnwhite commented 1 year ago

A permanent income growth factor of zero will break both HARK and our models in various ways, at least for models that normalize by permanent income. All of the representations will fail completely. Everything will work properly if someone approximates zero as PermGroFac = 0.0001.

On Tue, Sep 12, 2023 at 9:46 PM Christopher Llorracc Carroll < @.***> wrote:

Updating Seaborn import to avoid warning.

I wanted you to try to fix things so that the warning was no longer presented, regardless of the log level. That is, fix the bug, don't hide it. This presumably involves updating the version of seaborn that is installed in the HARK/binder/requirements.txt file; when you do that, you should trigger a test of whether that breaks any of the other DemARKs. If you are not sure how to do all of this, please ask Seb or Matt.

(Also, when I run your notebook, I am still getting the error message, so I'm puzzled you are not).

Figure 4 works now (Problem was that PermGroFac cannot be exactly zero).

Please post it as an issue in HARK that we should be able to accommodate PermGroFac of zero.

Writing all labels in Latex notation with lower values for time periods.

That was not the task. The task was to have identical variable names in the python code as in the plot labels. So the VARIABLE in python should not be B1, it should be b_1. (Please match the case in the lecture notes, where variables are lower case).

PS. I've turned this into a "draft" PR. Am trying to learn how draft PR's work.

— Reply to this email directly, view it on GitHub https://github.com/econ-ark/DemARK/pull/206#issuecomment-1716810445, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFNDLNNBGDR4BVEYLN3X2EGBFANCNFSM6AAAAAA4VJBU7E . You are receiving this because you are subscribed to this thread.Message ID: @.***>

AMonninger commented 1 year ago

Seaborn:

Variables in Latex:

PermGroFac:

AMonninger commented 1 year ago

@alanlujan91 Any idea why the checks are failing? It seems to fail because of something unrelated to the notebook I changed. I'm still using an old python version 3.8.8. Do I need to update to 3.11?

I also cannot fetch the latest version of DemArk... https://github.com/AMonninger/DemARK

alanlujan91 commented 1 year ago

I think the error is because @mnwhite refactored the check_conditions part of HARK, so when the tests pull the latest version of HARK, it makes a different notebook fail. You are correct that this is not related to your notebook, but it might have to be fixed (in a different PR) before yours can be merged.

alanlujan91 commented 1 year ago

@sbenthall you might be interested in this PR

llorracc commented 1 week ago

@ashishk87,

It seems that I asked Adrian for some more extensive changes last year. Could you look over the thread above and his PR and revise your PR so that it incorporates whatever is worthwhile about Adrian's edits in addition to your own?

llorracc commented 1 week ago

@AMonninger, @ashishk87 is working on merging his PR for this year with yours from last year to incorporate the improvements from both.

Yours is generating conflicts. Could you help Ashish resolve the conflicts and get your PR working, so he can meld them together?