PolicyEngine / policyengine-core

Core microsimulation engine for PolicyEngine models. Forked from OpenFisca-Core.
https://policyengine.github.io/policyengine-core
GNU Affero General Public License v3.0
14 stars 19 forks source link

Reforms not taking effect in Python package #251

Closed MaxGhenis closed 3 weeks ago

MaxGhenis commented 4 weeks ago

See https://colab.research.google.com/drive/1BMlqyPHVpWUOxDSRYeg2VGvp4o06nqA7?authuser=0#scrollTo=TkFJe-1OuyCv

image

Pinning to v1.0.0 made it work: https://colab.research.google.com/drive/1f2-FFRi52ZLVq0qZw_O6fV2BITKnVa8k?usp=sharing

I've tried with multiple other reforms, and gotten the same issue.

Seems like a caching issue, but haven't tried in -uk to check.

anth-volk commented 3 weeks ago

I receive an impact on the UK side when running the following, so I wonder if it's US-related:

from policyengine_uk import Microsimulation
from policyengine_core.reforms import Reform

baseline = Microsimulation()

baseline.calc("child_benefit", period=2024).sum()

reform = Reform.from_dict({
  "gov.hmrc.child_benefit.amount.eldest": {
    "2024-01-01.2100-12-31": 99
  }
}, country_id="uk")

reformed = Microsimulation(reform=reform)

reformed.calc("child_benefit", period=2024).sum()
anth-volk commented 3 weeks ago

I'm currently using a slightly outdated version of the US package. When I run the Colab code locally, I actually get an impact, with the calculated value increasingly quite a bit. I'm wondering if there's an issue in some recent change to US, and that this isn't caching-related. I'm running policyengine-us==1.35.0 and policyengine-core==2.21.8.

MaxGhenis commented 3 weeks ago

Huh, looking at the changelog I wonder if it's the unpinning of -core in 1.44.1: https://github.com/PolicyEngine/policyengine-us/blob/master/CHANGELOG.md

Could try running the notebook pinned just before/after that, e.g. in Colab web?

anth-volk commented 3 weeks ago

Not a bad idea. I just updated my local environment to -us at 1.56.1 and received the same non-erroneous outputs.

anth-volk commented 3 weeks ago

The issue appears to be -core. When I update locally to the newest version, the error occurs. The issue lies somewhere between 2.21.8 and 3.5.2.

anth-volk commented 3 weeks ago

And you're right, unpinning -core in -us==1.44.1 triggers the issue in Colab

MaxGhenis commented 3 weeks ago

Could you check if the UK example still works correctly? Since UK is also unpinned: https://github.com/PolicyEngine/policyengine-uk/blob/73aea6c46941f635d9b2b8e119d935db9e2f092f/setup.py#L36

anth-volk commented 3 weeks ago

UK is unpinned, but <3. When testing locally on US, 3.0.0 worked. As a result, it works correctly for me.

Incidentally, 3.1.0 and 3.2.0 completely crashed.

anth-volk commented 3 weeks ago

3.3.0 also completely crashes. 3.4.0 is where the bug is introduced, which makes me think we're having an issue loading flat-file datasets. I have to wonder if this doesn't also cause https://github.com/PolicyEngine/policyengine-uk/issues/928, but that's just a hunch, nothing more.

MaxGhenis commented 3 weeks ago

Oops, I mistook our latest version.

The parameter changes in https://github.com/PolicyEngine/policyengine-core/pull/225 (which bumped to 3.4.0) seem more likely than the flat file pieces.

anth-volk commented 3 weeks ago

Makes sense, thanks for raising

anth-volk commented 3 weeks ago

I believe the parameter changes in https://github.com/PolicyEngine/policyengine-core/pull/225 may have actually bumped to 3.1.0 based on the changelog. That said, I'm not sure when we unpinned -us vs. when we updated -core to 3.1.0, since running the default output from the Reproduce in Python block crashes if I use -core >3.0.0 and <3.4.0.

I couldn't find any issues in https://github.com/PolicyEngine/policyengine-core/pull/225 (which I theorize is 3.1.0, but can't test due to crashing), but did find the issue in https://github.com/PolicyEngine/policyengine-core/pull/230's simulation.py. As I said, I couldn't just test 3.1.0 vs. 3.2.0, but if I manually re-add the code from that file on top of the current master branch's code, the simulation does run properly.

I know we've had recurring issues with the reform_applied_after value, and it looks like this is one, as well.