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

Extensions to demographics.py #902

Merged
merged 16 commits into from
Feb 17, 2024
Merged

Conversation

jdebacker
Copy link
Member

This PR updates the demographics.py module to:

  1. Allow the user to have the population distribution from the initial period forward inferred from given fertility, mortality, and immigration rates (functionality to infer immigration from a given evolution of the population is retained, the user specifies what they want to do via arguments to the relevant function calls).
  2. Extends all series returned from the get_pop_objs() function over the full transition path of T+S periods (except those that apply only to a single period).

Addresses Issues #900 and #899

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1331b6a) 80.52% compared to head (8fbbb9c) 80.72%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
+ Coverage   80.52%   80.72%   +0.20%     
==========================================
  Files          19       19              
  Lines        4452     4499      +47     
==========================================
+ Hits         3585     3632      +47     
  Misses        867      867              
Flag Coverage Δ
unittests 80.72% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ogcore/demographics.py 84.31% <100.00%> (+2.84%) ⬆️

@jdebacker jdebacker marked this pull request as ready for review February 13, 2024 18:01
@rickecon
Copy link
Member

@jdebacker. This looks great. I ran the run_ogcore_example.py script on my local machine to make sure the TPI worked and converged. I will merge this as soon as the final tests pass.

You might consider updating the version to 0.11.2 in setup.py, ogcore/__init__.py, and adding a CHANGELOG.md entry that includes both the description at the top of the document and the GitHub compare hyperlink at the bottom of the document.

@jdebacker
Copy link
Member Author

@rickecon Hold off. I want to try to figure out why the first period resource constraint isn't being satisfied and I think it's coming from demographics.py. I'm still testing to find exact reason and will follow up here when I find it.

@rickecon
Copy link
Member

@jdebacker. Where is this error being manifest? Are you looking at the full TPI output after a run?

@jdebacker
Copy link
Member Author

@rickecon asks:

Where is this error being manifest? Are you looking at the full TPI output after a run?

Yes. I need set ENFORCE_SOLUTION_CHECKS = False in TPI.py so that there's not an assertion raised before the output is saved. But when I do that and then look at the resource constraint errors along the time path, the only period with an error in excess of our tolerance is the first period.


So let's just infer the pre-pop_dist from those.
"""
pop1 = pop_2D[0, :]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rickecon This line through 882 make the fix that solves the resource constraint error I was running into from this demographics module.

In TPI.py we make some implicit assumptions that the immigration rate in the pre-TPI period is equal to the the rate in the first period (see here) and that the mortality rates in the pre-TPI period is equal to the mortality rates in the first period (see here). This means that when we are using time varying mortality and fertility, we need to be very careful that everything lines up from not just the first period of TPI onward, but also from period -1 to the first model period.

The "fix" I'm making here is one option. It takes the assumptions embedded in TPI.py (that the period -1 mortality, fertility, and immigration rates are the same as in period 0) and uses that to solve for the period -1 population distribution given the population distribution in period 0. With this, the resource constraint issue when solving TPI is resolved.

The drawback to this approach is that the period -1 population is known from the data, but we don't use it. However, if we were to use the known period -1 population distribution and for things to all be consistent, we would need to pass the period -1 mortality, fertility, and immigration rates into the model. That means adding three new parameters to the dictionary returned from get_pop_objs and in the the Specifications object passed around OG-Core or having the same omega, imm_rates, rho, objects as we do now, but adding another period at the beginning (so they become length T+S+1). However, doing that would make the indexing of those arrays different from all others in the model and therefore confusing throughout the OG-Core code (i.e., imm_rates[0, :] would represent immigration rates in period -1, while Y[0] would represent GDP in period 0.)

I opted for the approach taken here because it keeps the rest of the code base more concise and clear and the approximation error is probably not very large. But I'm happy to chat about doing things differently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdebacker. Yes. I like this approach.

@rickecon
Copy link
Member

@jdebacker. There is currently one test failing in test_demographics.py::test_custom_series() .

@rickecon
Copy link
Member

@jdebacker. Thanks for this PR. Looks great. Merging.

@rickecon rickecon merged commit c9cc292 into PSLmodels:master Feb 17, 2024
11 checks passed
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.

3 participants