Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shallow copy entities when cloning TaxBenefitSystem instances #288

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

anth-volk
Copy link
Contributor

@anth-volk anth-volk commented Oct 5, 2024

Fixes #287.

When applying a reform, -core creates a new TaxBenefitSystem instance by using the TaxBenefitSystem.clone() method upon the current-law system. Previously, when doing so, it assigned the old entities by reference to the new TaxBenefitSystem instance, thereby merely linking them to the new system.

This was alright when running simulations that update variables or parameters, as those are left out of the clone, then added later. However, with reforms that contain a neutralize_variable call, the is_neutralized value is attached to the variable that is stored as part of the entity.

When a user on the front end would access the "Varying your earnings" component for a reform like the New York WFTC, which contains a neutralize_variable call, the following would occur:

  1. Default TaxBenefitSystem would calculate household net income under current law, properly calculating the neutralized variable without issues.
  2. TaxBenefitSystem.clone() would be used to create a new TBS. At this point, the entities are the same as in the default system, but linked to the new TBS. Its variable data would be incorrect, and thus incorrectly fed to the TBS's Population and each Population's Holder instances. However, this variable is neutralized correctly in the Simulation, and when calculating a variable, the Simulation checks for whether it's neutralized within the Simulation's TBS before trying to find a pre-calculated value.
  3. Axes would be added to the household and it would be again passed to the default TBS. At this point, no cloning is done, and the entities (and thus populations and their holders) would still be linked to the old (reform) TBS and the old neutralized variable described above in bullet point 2. The Simulation would attempt to calculate and check if the relevant population's holder contains a pre-calculated value. The holder would then would check if its variable is neutralized, think it is, and return an array of 0's.
  4. The same household with axes would now apply the reform, following a process very similar to bullet point 2.

This reform instead adds a Python shallow copy, preventing the entities from referring to stale TaxBenefitSystem instances.

This bug surfaced because of how the API calculates the "Varying your earnings" component, which would make it easier to test within the API. I will be adding a test there, which should be merged after this PR. Until then, this will remain in draft.

@anth-volk anth-volk merged commit 29cfc31 into PolicyEngine:master Oct 9, 2024
3 checks passed
@anth-volk anth-volk deleted the fix/varying_your_earnings branch October 9, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When calculating impacts of structural reforms, the line in the varying your earnings chart is not correct
1 participant