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

Changes to Gentle Introduction to HARK #1355

Merged
merged 9 commits into from
Dec 9, 2023

Conversation

sidd3888
Copy link
Collaborator

@sidd3888 sidd3888 commented Nov 5, 2023

Changes made according to points mentioned in #1354

Primarily made changes to the description of the models or fixed typos. Only thing I changed in the code was to generate the plot of the model with updated parameters and then removing the pertinent code, so that the image matches the description (also detailed in the issue).

@alanlujan91 sent you an email about one of details. Ready to merge from my end once that is addressed

Copy link

codecov bot commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1ad4731) 73.33% compared to head (53d54ac) 73.33%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1355   +/-   ##
=======================================
  Coverage   73.33%   73.33%           
=======================================
  Files          83       83           
  Lines       13596    13596           
=======================================
  Hits         9971     9971           
  Misses       3625     3625           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sidd3888 and others added 2 commits November 6, 2023 18:39
The code and the earlier model assumes $\Gamma_t$ to be a constant, so changed the notation in the later model as well
@alanlujan91 alanlujan91 linked an issue Nov 6, 2023 that may be closed by this pull request
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.

Great job! I made some notational changes but agree with all your changes.

@sidd3888
Copy link
Collaborator Author

sidd3888 commented Nov 6, 2023

A couple of things I wanted to clarify:

  • I don't know if this is an accident, but the first statement of survival probability is as a constant, not as $\aleph_t$, though setting it to a constant is clarified only two paragraphs down. Switching between $\aleph$ and $\aleph_t$ may appear notationally inconsistent
  • The use of $\mathsf{D}_t$ for probability of death: this notation is not used in the notebook after this. Should we be retaining it here? Just for this notebook, might suffice to say the problem ends with probability $1 - \aleph_t$
  • Everytime someone runs the NewExample model with $\Gamma = 0.01$, the graph would revert to one without the updated parameters, as it is left for the reader to change the parameters. I can slightly change the wording here to indicate that the graph would obey the characteristics described upon changing the parameters

Once these things are addressed, this is good to go (but you'll have to merge them, I can't)

@alanlujan91
Copy link
Member

@sidd3888 were you going to redo this one?

Added one sentence and a couple words, adjusted commas.
@mnwhite
Copy link
Contributor

mnwhite commented Dec 8, 2023

I adjusted a few words, including the age-varying notation in the first paragraph. I moved the sentence about death probability to be semicolon clause in the LivPrb sentence; it's less prominent now, as Siddarth is correct that it serves little purpose.

The existence of the reference to death probability might be a legacy of older notation. I think we used to use D for death probability and \cancel{D} for survival probability, making it more useful to actually mention death probability.

Some of the checks are now failing after adjusting text, which is surprising. I'll look into it.

@mnwhite
Copy link
Contributor

mnwhite commented Dec 8, 2023

It looks like it's because I cleared notebook output, but Sphinx relied on its existence. Will fix.

Output was mysteriously missing in some notebook cells; trying again for tests.
@mnwhite
Copy link
Contributor

mnwhite commented Dec 8, 2023

Ok, I'm baffled @sbenthall . I'm now getting an error from ruff-format, which is apparently unhappy with a coding style change I made-- I think I moved a comment or two. I can't get ruff to run on my computer, as it says that the notebook isn't a JSON file at all.

@alanlujan91
Copy link
Member

I think this should pass now, is it ready to merge @mnwhite ?

@mnwhite
Copy link
Contributor

mnwhite commented Dec 9, 2023 via email

@alanlujan91
Copy link
Member

first thing i did was merge all the changes in master branch
then update ruff
then ruff format .\examples\Gentle-Intro\Gentle-Intro-To-HARK.ipynb

@MridulS is there a way to let ruff make and push the changes needed instead of not passing?

@alanlujan91 alanlujan91 merged commit b6bc798 into econ-ark:master Dec 9, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modifications to A Gentle Introduction to HARK
3 participants