Updates for compatibility with latest OG-Core #54
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
===========================================
+ Coverage 36.64% 57.06% +20.41%
===========================================
Files 4 12 +8
Lines 191 361 +170
===========================================
+ Hits 70 206 +136
- Misses 121 155 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@SeaCelo in making these updates, I'm not longer able to retrieve the labor share data from the UN portal: I'm having trouble tracking down where this is in the UN API. Can you point me in the right direction or provide an updated link? Thanks! |
|
@jdebacker it will take me a couple of days. I’m traveling to look at colleges all weekend. I’ll dig into this and the other UN API issues when I return |
|
@jdebacker Not sure if FRED will be the source going forward. I checked the UN data and there is an issue with their latest update (the previous version has the data, but since April it is missing). I reached out to confirm what to expect and to find a permanent solution. |
|
@SeaCelo Thanks for following up. I did update to pull from FRED. Shall we just use that until the UN data become reliable again? |
|
@jdebacker I’ve confirmed that the series will no longer be available in the UN API after the ILO changed its criteria for reporting it. Let’s use FRED and I will explore using the ILO directly in the future. |
|
@jdebacker FRED has older data (2019) while ILO publishes 2021. ILO no longer publishes that specific country-indicator in the UN SDG data portal, but we can get it directly from the ILO site. The direct download url looks like this: and to get every year available by omitting the time variables: I can make changes to this PR to get the right value |
Changed the source of labor share of income (gamma) to ILOSTAT. This gets us the latest year available (currently 2021). Removed FRED as a source because it only gives us up to 2019.
|
@jdebacker I've added a suggested change for your review jdebacker#1 |
Update macro_params.py
|
@jdebacker I just tested it and it fails in two places. First it requires the line Second, it fails to get UN demographics data in ogcore: I'm going to spend some time looking at that code and the UN API |
|
The UN Population API is not returning any results. I've contacted them asking for an explanation and solution, but for now it isn't working. |
|
@jdebacker The UN population API is under maintenance and I don't have an ETA. I can merge this PR but it isn't getting past the issue in |
|
@SeaCelo Since things will not be working with or without this PR until the API is fixed, we can just let it sit here until then. |
|
The UN API is moving to require bearer tokens. This will need to be changed in OG-CORE (PSLmodels/OG-Core#931). Once the API and registration system is live I will implement the changes as a PR in OG-CORE, and this will inherit the fix. |
|
@SeaCelo The most recent commits to this PR set the default to download data if I don't have an API token, so I can't test, but you could do the following within the from ogcore.parameters import Specification
from ogzaf.calibrate import Calibration
p = Specifications()
c =Calibration(p, estimate_pop=True)You can then check that the downloaded data is in After that, I can adjust the code a bit to have an option to use these cached data, with some flexibility to allow users to pull different year windows from it (FYI, if this works for you, it'll be downloaded the full series out to 2100). Let me know how this works for you. |
|
@jdebacker I will test this soon. In the meantime I have requested tokens for you and @rickecon. |
|
@jdebacker Good news that I was able to run this code and get results from the API. But this required other adjustments to OG-Core's This is the draft PR in OG-Core with more information (PSLmodels/OG-Core#934). I created a PR to upload the files here: jdebacker#2 Let me know if you need me to test anything else until you get your own token. |
|
@SeaCelo and @rickecon this PR is ready to review and merge. It gets OG-ZAF working with the latest OG-Core and removes a bunch of unused lines of code. Before replicating these files to other country repos it would be good to work on 'macro_params.py'. I'll open an issue later outlining suggested changes there. |
|
@jdebacker. I am working on this PR right now. Doing a deep dive. Should have comments to you by 1:30p ET. |
|
@jdebacker. I just submitted a PR to your branch for this PR. There is an issue in |
Updates to Jason's PR branch from Rick
|
@jdebacker. I get the same error as you now running |
|
@jdebacker. I think I've ruled out the issue resulting from an updated version of NumPy. I ran OG-USA on my So I think we need to look directly at what this script is doing at line 88 of |
|
@rickecon I still don't understand why, but it has to do with how I was trying to update the tax function parameters in the OG-ZAF run script: p.update_specifications(
{
"cit_rate": [[0.27]],
"tax_func_type": "linear",
"age_specific": False,
"etr_params": [[[0.22]]],
"mtrx_params": [[[0.31]]],
"mtry_params": [[[0.25]]],
"tau_c": [[0.15]],
}
)When I comment out the ETR and MTR parameters in the dictionary above, Those ETR and MTR parameters go through validation and it looks like they are in the For now, perhaps we just go with the current run script, but in another PR we'll need to update the IIT tax rates for ZAF. |
|
@jdebacker. All good. Everything passing on my machine. Let's circle back to fix the tax functions. Merging now. |
|
@jdebacker. BTW. The GH Action CI tests after merge (on push) were failing on the Linux tests at the CodeCov stage. It looks like we just had a bad CODECOV_TOKEN in the settings. I reset that token, and everything has now passed. |
This PR makes updates to OG-ZAF to work with the latest OG-Core.