PSLmodels / OG-UK

An overlapping generations model to simulate fiscal policy the United Kingdom.
https://pslmodels.github.io/OG-UK
Other
6 stars 7 forks source link

Update openfisca-uk usage #11

Closed nikhilwoodruff closed 3 years ago

nikhilwoodruff commented 3 years ago

OpenFisca-UK just had a considerable update merged, which uses a new interface (to make loading data from different surveys and years much easier, as well as performance improvements). I've adjusted the code here that accesses openfisca-uk to use the new API (largely just syntax changes). A few comments/questions on the pre-existing code, the semantics of which I haven't changed:

codecov-commenter commented 3 years ago

Codecov Report

Merging #11 (0b9d8b7) into main (55f9f95) will decrease coverage by 0.13%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   51.76%   51.63%   -0.14%     
==========================================
  Files           5        5              
  Lines         736      736              
==========================================
- Hits          381      380       -1     
- Misses        355      356       +1     
Flag Coverage Δ
unittests 51.63% <100.00%> (-0.14%) :arrow_down:

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

Impacted Files Coverage Δ
og_uk_calibrate/get_micro_data.py 57.14% <100.00%> (ø)
og_uk_calibrate/tests/test_get_micro_data.py 85.71% <100.00%> (ø)
og_uk_calibrate/tests/test_txfunc.py 70.68% <0.00%> (-0.58%) :arrow_down:

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 55f9f95...0b9d8b7. Read the comment docs.

nikhilwoodruff commented 3 years ago

@jdebacker @rickecon not really sure why this is specifically failing on 3.9? I've only really changed the API specifics, the inputs and outputs to the openfisca code should be identical. Could be an openfisca-uk issue.

jdebacker commented 3 years ago

@nikhilwoodruff it's nothing you did. The PR adding openfisca UK also failed 3.9 but was merged before tests completed.

I don't know why it fails but you can remove 3.9 from test matrix if you like. I think that's ok since openfisca only tested on 3.7.

rickecon commented 3 years ago

@jdebacker's OpenFisca-UK Integration PR #2 to this repo was passing all tests when I merged it. And build_and_test.yml tested Python 3.7, 3.8, and 3.9. I think @nikhilwoodruff is right that this issue was introduced with the update to OpenFisca-UK. But I think it is fine for now to remove Python 3.9 from the build_and_test.yml.

rickecon commented 3 years ago

@nikhilwoodruff @jdebacker. I have reviewed this PR, and it looks good to me. Any further changes from either of you two?

nikhilwoodruff commented 3 years ago

Thanks @rickecon - nothing further to add here from me.