jdebacker
commented
1 year ago @harisjoshi Your latest changes look great! I think the next step is to update parameters.py. I will follow up with details about that.
Closed harisjoshi closed 1 year ago
jdebacker
commented
1 year ago @harisjoshi Your latest changes look great! I think the next step is to update parameters.py. I will follow up with details about that.
jdebacker
commented
1 year ago @harisjoshi I just opened a PR to this branch of yours here. If you merge that in the changes will be reflected on your branch and in this PR. With that, I think we'll have completed what we need for this addition to OG-Core!
codecov-commenter
commented
1 year ago Merging #871 (bd11970) into master (5a770b5) will decrease coverage by
0.04%. Report is 20 commits behind head on master. The diff coverage is72.72%.
jdebacker
commented
1 year ago @harijoshi This is very close. It looks like all units tests are passing (which I also confirmed on my local machine), but we need to update the formatting to be consistent with the rest of this project.
Can you, in your terminal, navigate to the OG-Core to level directory. Then run: black -l 79 ./ to format the files in this repository that need formatting?
After that you can use git status -uno to see what files have changed. Got ahead and add these changed file with git add -u, and then commit and push the changes to this branch. Thanks!
@rickecon Can you review this PR? I've worked with @harisjoshi on this and it looks good to me (and ready to merge after the formatting changes are made).
jdebacker
commented
1 year ago @harisjoshi Thanks for updating the formatting, unfortunately the PR with the updated formatting lost the changes merged in from my PR.
I think what you need to do is a git pull origin to bring down the remote changes on your branch from the merge of my PR. Then with those changes pulled, you'll want to merge them in with git merge origin/mortrates.
Once merged, run Black on more time: black -l 79 ./.
Then add and commit the changed and push here.
If you want, I can try to find some time to jump on a Zoom with you Tuesday afternoon/evening or Wednesday afternoon to walk through this with you.
harisjoshi
commented
1 year ago @jdebacker I will try to bring down the remote changes from my branch from the PR over the weekend. If I do have trouble, would a quick call next week be feasible?
rickecon
commented
1 year ago @jdebacker @harisjoshi . I am trying to pull this branch from @harisjoshi to test it out on my machine, and it is telling me that I have divergent branches. I have updated my master branch to the main repo master. Then I used the command line branch creation and pull instructions.
git checkout -b harisjoshi-mortrates master
git pull https://github.com/harisjoshi/OG-Core.git mortratesI get the following error, which usually means that the PR branch has not incorporated whatever are the current changes in the master repo branch.
(ogcore-dev) richardevans@Richards-MacBook-Pro-2 OG-Core % git pull https://github.com/harisjoshi/OG-Core.git mortrates
From https://github.com/harisjoshi/OG-Core
* branch mortrates -> FETCH_HEAD
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint: git config pull.rebase false # merge
hint: git config pull.rebase true # rebase
hint: git config pull.ff only # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.@rickecon Thanks for noticing this.
@harisjoshi Can you do a git fetch upstream and then a git merge upstream/master while in this branch? Let me know if there are any conflicts on the merge and we can work through them. Once that's done, you might run black again (black -l 79 ./ from OG-Core directory) then commit and push the changes.
rickecon
commented
1 year ago @jdebacker @harisjoshi. I went ahead and just git fetched the branch.
git checkout -b harisjoshi-mortrates master
git fetch https://github.com/harisjoshi/OG-Core.git mortrates
git merge FETCH_HEADI looked at the changed files from this PR on my machine in my new branch harisjoshi-mortrates to make sure my branch has all the updates. I am running run_ogcore_example.py right now. Seems to be working well so far.
rickecon
commented
1 year ago @jdebacker @harisjoshi. Here are some suggestions and questions about this PR. First let me state my understanding of what it does. It allows the age-specific mortality rates to also vary over time, making the rho parameter object a 2 dimensional object (S-element list nested in a list) instead of its original one dimensional object (S elements in a list).
surv_rate object in default_parameters.json to also be a two-dimensional object. It is defined simply as 1 - rho (see line 684 of demographics.py in OG-USA), so we should make sure it corresponds to the values in the rho object in default_parameters.json in OG-Core as well as in OG-USA. And both rho and surv_rate will need to be updated in the og[cntry]_default_parameters.json and demographics.py files in all the country repositories.surv_rate object from demographics.py and og[cntry]_default_parameters.json in all the country repositories, as well as from all of its instances in OG-Core (one default_parameters.json, four parameters.py, one testing_params.json, one create_params_inputs.pkl. We could do this by simply hard coding surv_rate into parameters.py in OG-Core.rickecon
commented
1 year ago @jdebacker @harisjoshi. Everything ran great in run_ogcore_example.py--baseline steady state and transition path and reform steady state and transition path.
jdebacker
commented
1 year ago @rickecon -- thanks for your review! Let me respond to each item.
First let me state my understanding of what it does. It allows the age-specific mortality rates to also vary over time, making the rho parameter object a 2 dimensional object (S-element list nested in a list) instead of its original one dimensional object (S elements in a list).
That's right. Although I'll note that the object is turned into 2 dimensional NumPy array upload loading into the Specifications object.
You should probably update the surv_rate object in default_parameters.json to also be a two-dimensional object. It is defined simply as 1 - rho (see line 684 of demographics.py in OG-USA), so we should make sure it corresponds to the values in the rho object in default_parameters.json in OG-Core as well as in OG-USA.
Good call. This should be updated here for consistency (although we'll make changes to this in the future - see following responses). @harisjoshi Can you make the required change to the default_parameters.json file? This would be updating the surv_rate parameter so that "number_dims": 2 and adding a set of square brackets around the current list?
And both rho and surv_rate will need to be updated in the og[cntry]_default_parameters.json and demographics.py files in all the country repositories.
Yes, this will have to be done in subsequent PRs in those country calibration repositories.
We might consider removing the surv_rate object from demographics.py and og[cntry]_default_parameters.json in all the country repositories, as well as from all of its instances in OG-Core (one default_parameters.json, four parameters.py, one testing_params.json, one create_params_inputs.pkl. We could do this by simply hard coding surv_rate into parameters.py in OG-Core.
100% agree with this. It's only used to create the population distribution in the case of constant_demographics=True and is redundant with rho. I would say we do this in a future PR, as I think there are several changes we an make together that involve the demographics modules (namely, moving demographics.py to OG-Core to get rid of redundancies.
rickecon
commented
1 year ago @jdebacker and @harisjoshi. I think this is harder than you think. I don't think you can just update surv_rate in default_parameters.json (set dim=2, and extra square bracket around data). Because surv_rate gets imported from OG-USA's demographics.py file as a numpy array, that whole section also has to be changed. Furthermore, I think this breaks some of the tests that use surv_rate. I think this will require a bigger PR than we expect.
I started doing this in @harisjoshi 's branch and was going to submit a PR to him. But I kept running into errors in the testing suite due to stuff saved in testing_params.json and create_params_inputs.pkl. Maybe we should just merge this and make an issue reminiding us to fix and take out the surv_rate object. What do you think?
jdebacker
commented
1 year ago I think this is harder than you think. I don't think you can just update surv_rate in default_parameters.json (set dim=2, and extra square bracket around data). Because surv_rate gets imported from OG-USA's demographics.py file as a numpy array, that whole section also has to be changed.
Yes, this PR will break all the country calibrations as there will need to be updates in those country's demographics.py modules. My recommended workflow would be to:
surv_rate parameter.rho and removing surv_rate from the output of demographics.pydemographics.py, where the module includes a country code parameter and where the functions are updated to take the time varying mortality and fertility rates from the UNdemographics.py and use ogcore.demographics insteadDoes this sounds like a reasonable plan?
rickecon
commented
1 year ago @jdebacker. Your plan above sounds great.
@harisjoshi. Thanks for this PR. I am merging it now.
This pull request is going to update OG core to make time-varying mortality rates.