Fix Variable.clone() to preserve update_variable-inherited attributes (#502)#511
Draft
anth-volk wants to merge 6 commits into
Draft
Fix Variable.clone() to preserve update_variable-inherited attributes (#502)#511anth-volk wants to merge 6 commits into
anth-volk wants to merge 6 commits into
Conversation
Lock the existing clone() contract for normal variables (attributes preserved, same class, independent mutable containers) before fixing #502. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) Cloning a variable registered via a reform's update_variable currently raises "Missing attribute 'value_type'" because clone() re-runs __init__ without the baseline. Documented as strict xfail (label-only and adds-only overrides) until the fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#502) clone() re-ran __init__ with baseline_variable=None, dropping every attribute a reform's update_variable merged in from the baseline (value_type, entity, formulas, ...) and raising "Missing attribute 'value_type'". Cloning a reformed tax-benefit system — which the YAML test runner and branch calculations do for every variable — therefore crashed. Clone now mirrors TaxBenefitSystem.clone(): empty_clone + __dict__ copy, preserving the variable's merged state, with formulas/metadata copied so the clone stays independent. The two #502 regression tests are un-xfailed. Fixes #502. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an end-to-end test that clones a whole reformed TaxBenefitSystem (the path the YAML test runner and branch calculations take), plus a test that a runtime-set attribute (metadata) is preserved by clone() and stays independent of the original. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Docstring now states that formulas and metadata are copied (adds/subtracts are shared, as before) and that baseline_variable is retained. Add tests for the real reform paths that clone under the hood — get_neutralized_variable and get_annualized_variable of an update_variable variable — plus an Enum variable clone and an assertion documenting adds sharing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #502
Summary
Variable.clone()re-ran__init__withbaseline_variable=None, dropping every attribute a reform'supdate_variablemerged in from the baseline (value_type,entity,formulas, …) and raisingValueError: Missing attribute 'value_type'. BecauseTaxBenefitSystem.clone()clones every variable — and the YAML test runner + branch calculations clone the system — any reform usingupdate_variablecrashed on clone.Fix
clone()now mirrorsTaxBenefitSystem.clone():empty_clone+ a__dict__copy, preserving the variable's merged (baseline + override) state instead of re-deriving it.formulas/metadataare copied so the clone stays independent of the original (the prior__init__-based clone produced fresh containers). Not re-running__init__also avoids a spuriouscheck_computation_modes()re-validation.Verified safe for all three
Variable.clone()callers (get_annualised_variable,get_neutralized_variable,TaxBenefitSystem.clone) — they reassign containers or mutate scalars, never mutateformulas/metadatain place. No callers in policyengine-us/uk.Commits (TDD)
clone()(green on unchanged code).xfail(label-only + adds-only overrides).TaxBenefitSystem(the test-runner/branch path) + a runtime-attribute preservation/independence test.Verification
tests/core/variables/test_variable_clone.py: 7 tests.tests/core: 658 passed, 1 skipped, 1 xfailed (no regressions).🤖 Generated with Claude Code