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

Time varying unemployment probability and income #1112

Merged
merged 6 commits into from
Feb 16, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Feb 14, 2022

This PR allows CondIndShock and its derivatives to use age-varying unemployment probabilities and unemployment incomes.

This can be done passing lists as the arguments for UnempPrb and ÌncUnemp.

  • Tests for new functionality/models or Tests to reproduce the bug-fix in code.
  • Updated documentation of features that add new functionality.
  • Update CHANGELOG.md with major/minor changes.

@Mv77
Copy link
Contributor Author

Mv77 commented Feb 14, 2022

Addresses #1015

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #1112 (de40482) into master (ef8d8fd) will decrease coverage by 0.02%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
- Coverage   73.85%   73.83%   -0.03%     
==========================================
  Files          69       69              
  Lines       10631    10636       +5     
==========================================
+ Hits         7852     7853       +1     
- Misses       2779     2783       +4     
Impacted Files Coverage Δ
...RK/ConsumptionSaving/tests/test_ConsMarkovModel.py 86.56% <ø> (ø)
HARK/ConsumptionSaving/tests/test_modelInits.py 81.39% <ø> (ø)
HARK/ConsumptionSaving/ConsIndShockModel.py 85.82% <42.85%> (-0.39%) ⬇️

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 ef8d8fd...de40482. Read the comment docs.

@llorracc
Copy link
Collaborator

@alanlujan91, can you review this because it's related to a couple of things you are doing.

@Mv77, @alanlujan91, @sbenthall:

We should have a generic mechanism for making any and all inputs of this kind time-varying. (Or, as we have agreed to call it henceforth, age-varying. Actually, I thought we already did, which was to identify the item in the time_vary list of variables. Is the problem that, in order to be eligible to go into time_vary the type needs to be a list, and some things which we did not conceive as age-varying were scalars or some other non-list type?

@sbenthall
Copy link
Contributor

Adding a variable name to a list called time_varying does not, in itself, make the code function in a way such that, say, a list of values is interpreted in all parts of the program as a time-varying parameter.

There are many open tickets that are about make a more unified and coherent approach to time-varying parameters. #95 #664 #890 #991 among others

@alanlujan91
Copy link
Member

@llorracc I think that's correct. If we want a variable to be time-varying in some cases, even if it's in few cases and the main use case is time-fixed, the variable has to be added to time_vary and given as a list [x] of one element. Something we have been thinking about with the AgentPopulation in SHARKFin is how to flexibly infer time (and agent) variability depending on the argument passed.

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.

Changes look good to me!

A quick suggestion is that sometimes the types of parameters might be ints. I saw that you had to change this in some tests to make them floats. I assume this was for the type checks?

Something that can help with type checks is isinstance(x, (int, float)) to catch any scalar parameter.

@Mv77
Copy link
Contributor Author

Mv77 commented Feb 14, 2022

Thank you @alanlujan91!

Yes, you are right about the type issue. But I thought it might be a good opportunity to enforce probabilities being floats.@sbenthall, do you have an opinion on this?

@sbenthall
Copy link
Contributor

I don't see any harm in enforcing that the probabilities in this case are floats.

I wonder how that subtlety can be best documented.

I agree with @llorracc that the main issue we are bumping into is that there's something non-generic about how this (rather core) model is configured. I don't think addressing that architectural concern is within the scope of this PR. #1111 might be a good place to discuss it in more detail.

@Mv77
Copy link
Contributor Author

Mv77 commented Feb 14, 2022

Great, this is ready to merge then.

Thank you all!

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