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

'NewbornTransShk: Option if Newborns get transitory shock' #1126

Merged
merged 5 commits into from
Mar 23, 2022

Conversation

AMonninger
Copy link
Collaborator

Pull request for issue:
Newborns' transitory shocks to income #1125

I added an aditional variable: NewbornTransShk which is by default true. If so, Newborn consumers do NOT get a transitory shock in the first period, only a permanent one.
By setting it 'False' Newborn consumers Do get a transitory shock as well.

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2022

Codecov Report

Merging #1126 (a612bd4) into master (600372f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1126   +/-   ##
=======================================
  Coverage   73.95%   73.96%           
=======================================
  Files          70       70           
  Lines       10759    10761    +2     
=======================================
+ Hits         7957     7959    +2     
  Misses       2802     2802           
Impacted Files Coverage Δ
HARK/ConsumptionSaving/ConsIndShockModel.py 85.86% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 600372f...a612bd4. Read the comment docs.

@Mv77
Copy link
Contributor

Mv77 commented Mar 18, 2022

This is neat and passes tests.

A minor comment:
If I see an option called NewbornTranShk and set it to true, I'd expect newborns to get shocks. I'd just suggest to make it so that true->shocks and false->no-shocks, making the default false.

@llorracc
Copy link
Collaborator

I agree with Mateo - true means they get a shock.

Mateo, could you merge this when Adrian revises?

@Mv77
Copy link
Contributor

Mv77 commented Mar 21, 2022

I'd be happy to, but I don't have merge permissions.

@AMonninger
Copy link
Collaborator Author

Just pushed the changes.
Is there a way to put it in the documentation as well?

@Mv77
Copy link
Contributor

Mv77 commented Mar 21, 2022

Yes, you just add one short line to the changelog, a bullet point below "minor changes" saying what you did and linking to the PR.

These changes would be made in a commit to this same pr.

@Mv77
Copy link
Contributor

Mv77 commented Mar 22, 2022

This now looks good. Thanks a lot @AMonninger .

@llorracc, I do not have the permissions to either approve the remaining tests (workflows?) or merge.

@llorracc
Copy link
Collaborator

@AMonninger, for the documentation, you should also add something to the docstring listing the variable as an input and briefly explaining what it does.

Once you have done that and it passes all tests, I will merge.

@AMonninger
Copy link
Collaborator Author

I added it in the 'get_shocks' function as it is the only function affected by the change. Is this what you meant?

@llorracc
Copy link
Collaborator

@AMonninger,

Yes, I now see you've added a docstring at the right place. However, see the syntax for optional boolean variables used elsewhere (I think you need it to say something like ": Boolean, Optional" to mark it in the documentation as optional. Also, having done that, please rewrite the description accordingly. (See other examples of optional variables for templates).

@llorracc llorracc merged commit d1a4809 into econ-ark:master Mar 23, 2022
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
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.

5 participants