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

Fisher two period #206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

AMonninger
Copy link
Contributor

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
Copy link
Contributor Author

@llorracc can you have a look at it?

@llorracc llorracc marked this pull request as draft September 13, 2023 01:18
@llorracc
Copy link
Collaborator

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
Copy link
Collaborator

mnwhite commented Sep 13, 2023 via email

@AMonninger
Copy link
Contributor Author

Seaborn:

plt.style.use("seaborn-v0_8-darkgrid")
  • Option 2: We use the style from seaborn directly
import seaborn as sns
sns.set_style("darkgrid")
  • In the latest push I used option 2

Variables in Latex:

  • I changed variable names now accordingly (B1 changed to b_1)
  • I changed many variable names to lower cases and kept only aggregate variables with an upper case (CRRA, R)

PermGroFac:

  • In my earlier push, I changed PermGroFac from 0 to 0.0001 to avoid a break in the code (as @mnwhite suggested)
  • If you have very strong feelings that a user should be able to declare PermGroFac of 0, we could add two lines of code replacing any input of PermGroFac = 0.0 to PermGroFac = 0.0001

@AMonninger
Copy link
Contributor Author

AMonninger commented Sep 18, 2023

@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
Copy link
Member

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
Copy link
Member

@sbenthall you might be interested in this PR

@alanlujan91 alanlujan91 marked this pull request as ready for review February 6, 2024 15:03
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.

None yet

4 participants