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

OpenFisca-UK integration #2

Closed jdebacker closed 3 years ago

jdebacker commented 3 years ago

This PR integrates OpenFisca-UK into oguk_calibration. This microsimulation model will provide the underlying data on marginal and effective tax rates that will go into the UK calibration of the OG model.

jdebacker commented 3 years ago

@nikhilwoodruff Can you look at the testing failures and see if you can identify why the conflict in setting up the environment arises? The issue is described here:

  The conflict is caused by:
1093
      The user requested openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git)
1094
      openfisca-uk 0.2.2 depends on openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core)
  ERROR: Cannot install -r /home/runner/work/OG-UK-Calibration/OG-UK-Calibration/condaenv.1lzvviqe.requirements.txt (line 5) and openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git) because these package versions have conflicting dependencies.
1128
  ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/user_guide/#fixing-conflicting-dependencies
rickecon commented 3 years ago

@jdebacker and @nikhilwoodruff. As @jdebacker notes above, the error states:

The conflict is caused by:
      The user requested openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core.git)
      openfisca-uk 0.2.2 depends on openfisca-core 35.0.0 (from git+https://github.com/nikhilwoodruff/openfisca-core)

  To fix this you could try to:
  1. loosen the range of package versions you've specified
  2. remove package versions to allow pip attempt to solve the dependency conflict

This is going to take some detective work to figure out what the conflict is. It sounds like it is a conflict between some package in our environment.yml and the openfisca-core 35.0.0.

I tried fixing the Python version in environment.yml to python==3.7.7 and removing the ability to accommodate Python 3.8 and 3.9 in setup.py in a PR that I submitted to @jdebacker's branch. But that gave the same error. That PR is now closed and not merged.

nikhilwoodruff commented 3 years ago

Thanks @rickecon and @jdebacker it's possible openfisca-uk has some lack of support (purely because of not specifying different package versions) for Python above 3.7 - I'll have a look and see if I can fix it. Not this - see below

nikhilwoodruff commented 3 years ago

Just changed the openfisca-uk dependency for microdf (pslmodels/... PSLmodels/...) which should at least fix the current error.

codecov-commenter commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@02cc204). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #2   +/-   ##
=======================================
  Coverage        ?   51.76%           
=======================================
  Files           ?        5           
  Lines           ?      736           
  Branches        ?        0           
=======================================
  Hits            ?      381           
  Misses          ?      355           
  Partials        ?        0           
Flag Coverage Δ
unittests 51.76% <0.00%> (?)

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


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 02cc204...8b94be8. Read the comment docs.

jdebacker commented 3 years ago

Thanks @nikhilwoodruff, tests on 3.7 appear to pass now!

rickecon commented 3 years ago

I have reviewed all the changes. This looks good to me. @jdebacker are you ready for me to merge this? Thanks, @nikhilwoodruff for the help.

jdebacker commented 3 years ago

@rickecon Yes, this PR is ready to merge. Thanks for reviewing!