From 9e5bc21eae3e8330bfca600c8ec7c791dfa11192 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 1 Oct 2026 09:57:46 -0400 Subject: [PATCH 1/6] Carry over only values stored at the variable's definition period Since 3.24.0 (#457, "bug H1") auto-carry-over took the latest-starting known period of any unit. A yearly variable also caches month values: calculate_divide stores a twelfth of the year's value. So once any month after January had been requested, a yearly input known only for that year carried a twelfth of its value into every later year. policyengine-us requests age by month through monthly_age, so a Microsimulation that calculated 2024 and then 2025 on a single-year dataset aged everyone to a twelfth of their age in 2025. On a 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. Filter known periods to the definition period's unit before carrying over, as the uprating path beside it already does. A MONTH variable's year-sum cache (calculate_add) is excluded the same way. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 1 + policyengine_core/simulations/simulation.py | 23 +- .../core/test_carry_over_definition_period.py | 216 ++++++++++++++++++ 3 files changed, 234 insertions(+), 6 deletions(-) create mode 100644 changelog.d/fix-carry-over-definition-period.fixed.md create mode 100644 tests/core/test_carry_over_definition_period.py diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md new file mode 100644 index 000000000..7f3faba30 --- /dev/null +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -0,0 +1 @@ +Auto-carry-over now reads only values stored at the variable's own definition period. Since 3.24.0 it could pick a month value cached for a yearly variable (a twelfth of the year's value), so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 566407a59..ae1071e07 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -865,12 +865,22 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # If no result, use the default value and cache it if array is None: # Check if the variable has a previously defined value - known_periods = holder.get_known_periods() + # Only a value stored at the variable's own definition period + # can stand in for another period. A YEAR variable also caches + # month values (``calculate_divide`` stores a twelfth of the + # year's value, the STOCK path the whole value) and a MONTH + # variable caches year values (``calculate_add`` stores the sum + # of the months). Those are derived from a stored value, so + # carrying one forward would rescale the variable. + known_periods = [ + known_period + for known_period in holder.get_known_periods() + if known_period.unit == variable.definition_period + ] earlier_known_periods = [ known_period for known_period in known_periods - if known_period.unit == variable.definition_period - and known_period.start < period.start + if known_period.start < period.start ] if variable.uprating is not None and len(earlier_known_periods) > 0: # Take the latest period from the filtered list itself. @@ -915,9 +925,10 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: ): # Variables with a calculate-output property specify # Sort by period.start (temporal order). Sorting Period - # tuples lexicographically puts "year" before "month" - # alphabetically, so a known "2023" annual value would - # win over a later "2024-06" monthly value (bug H1). + # tuples lexicographically compares their unit strings + # first (bug H1). Every period here has the definition + # period's unit, so a twelfth of a year's value cached at + # "2024-12" cannot stand in for "2025". last_known_period = max(known_periods, key=lambda p: p.start) if last_known_period.start > period.start: return holder.default_array() diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py new file mode 100644 index 000000000..72afa184f --- /dev/null +++ b/tests/core/test_carry_over_definition_period.py @@ -0,0 +1,216 @@ +"""Auto-carry-over reads only values stored at the variable's definition period. + +A YEAR variable also caches month values: ``calculate_divide`` stores a +twelfth of the year's value, and the STOCK path stores the whole value. A +MONTH variable caches year values: ``calculate_add`` stores the sum of its +months. Since 3.24.0 auto-carry-over took the latest-starting known period +of any unit, so once any month of 2024 had been requested, a YEAR input +known only for 2024 carried a twelfth of its value into 2025. + +policyengine-us computes ``monthly_age`` from ``age`` one month at a time. +A microsimulation that calculated 2024 and then 2025 on a single-year +dataset therefore aged every person to a twelfth of their age in 2025. On a +3,000-household Enhanced CPS subsample, 2025 federal income tax came to +$31.6bn, against $1,846.9bn in a simulation that calculated only 2025. + +The invariant pinned here: what a simulation calculates for a later period +does not depend on which earlier periods it calculated first. Each case +compares against a fresh simulation that calculates only the later period. +""" + +from __future__ import annotations + +import numpy as np +import pytest + +from policyengine_core import periods +from policyengine_core.country_template import ( + CountryTaxBenefitSystem, + entities, + situation_examples, +) +from policyengine_core.simulations import SimulationBuilder +from policyengine_core.variables import QuantityType, Variable + +BASE_YEAR = 2024 +MONTHS = [f"{BASE_YEAR}-{month:02d}" for month in range(1, 13)] + + +class carried_flow(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + label = "Yearly flow input with no formula and no uprating" + + +class carried_stock(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + quantity_type = QuantityType.STOCK + label = "Yearly stock input with no formula and no uprating" + + +class carried_count(Variable): + value_type = int + entity = entities.Person + definition_period = periods.YEAR + label = "Yearly integer input with no formula and no uprating" + + +class carried_flag(Variable): + value_type = bool + entity = entities.Person + definition_period = periods.YEAR + label = "Yearly boolean input with no formula and no uprating" + + +class carried_monthly_flow(Variable): + value_type = float + entity = entities.Person + definition_period = periods.MONTH + label = "Monthly flow input with no formula and no uprating" + + +YEAR_INPUTS = { + "carried_flow": np.array([40.0, 6.0]), + "carried_stock": np.array([40.0, 6.0]), + "carried_count": np.array([40, 6]), + "carried_flag": np.array([True, False]), +} + + +@pytest.fixture(scope="module") +def system(): + system = CountryTaxBenefitSystem() + system.auto_carry_over_input_variables = True + system.add_variables( + carried_flow, + carried_stock, + carried_count, + carried_flag, + carried_monthly_flow, + ) + return system + + +def _simulation(system, inputs): + simulation = SimulationBuilder().build_from_entities( + system, situation_examples.couple + ) + for variable, values in inputs.items(): + for period, array in values.items(): + simulation.set_input(variable, period, array) + return simulation + + +# Earlier requests that leave month (or year) caches behind before a later +# period is calculated. +EARLIER_REQUESTS = [ + [], + [str(BASE_YEAR)], + [MONTHS[0]], + [MONTHS[5]], + [MONTHS[-1]], + MONTHS, + [MONTHS[-1], str(BASE_YEAR)], + [str(BASE_YEAR), *MONTHS], +] +LATER_PERIODS = ["2025", "2027", "2025-03", "2027-12"] + + +def test_month_cache_of_year_input_does_not_carry_into_next_year(system): + """The policyengine-us ``monthly_age`` sequence, on a synthetic variable.""" + simulation = _simulation(system, {"carried_flow": {BASE_YEAR: [40.0, 6.0]}}) + + december = simulation.calculate("carried_flow", MONTHS[-1]) + np.testing.assert_allclose(december, [40.0 / 12, 6.0 / 12]) + + np.testing.assert_array_equal( + simulation.calculate("carried_flow", BASE_YEAR + 1), [40.0, 6.0] + ) + + +@pytest.mark.parametrize("later_period", LATER_PERIODS) +@pytest.mark.parametrize( + "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" +) +@pytest.mark.parametrize("variable", sorted(YEAR_INPUTS)) +def test_year_input_later_period_does_not_depend_on_earlier_requests( + system, variable, earlier, later_period +): + inputs = {variable: {BASE_YEAR: YEAR_INPUTS[variable]}} + fresh = _simulation(system, inputs).calculate(variable, later_period) + + simulation = _simulation(system, inputs) + for period in earlier: + simulation.calculate(variable, period) + result = simulation.calculate(variable, later_period) + + np.testing.assert_array_equal(result, fresh) + if periods.period(later_period).unit == periods.YEAR: + # Carry-over of an input is the input itself. + np.testing.assert_array_equal(result, YEAR_INPUTS[variable]) + + +@pytest.mark.parametrize("later_period", LATER_PERIODS) +@pytest.mark.parametrize( + "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" +) +def test_year_input_branch_does_not_depend_on_earlier_requests( + system, earlier, later_period +): + """A branch forked after the earlier requests clones their month caches.""" + inputs = {"carried_flow": {BASE_YEAR: YEAR_INPUTS["carried_flow"]}} + fresh = _simulation(system, inputs).calculate("carried_flow", later_period) + + simulation = _simulation(system, inputs) + for period in earlier: + simulation.calculate("carried_flow", period) + branch = simulation.get_branch("itemizing") + + np.testing.assert_array_equal(branch.calculate("carried_flow", later_period), fresh) + np.testing.assert_array_equal( + simulation.calculate("carried_flow", later_period), fresh + ) + + +MONTHLY_INPUT_PATTERNS = { + "every_month": {month: [float(i + 1), 100.0 + i] for i, month in enumerate(MONTHS)}, + "march_only": {MONTHS[2]: [3.0, 103.0]}, + "december_only": {MONTHS[-1]: [12.0, 112.0]}, +} + + +@pytest.mark.parametrize("later_period", ["2025-01", "2025-07", "2026"]) +@pytest.mark.parametrize( + "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" +) +@pytest.mark.parametrize("pattern", sorted(MONTHLY_INPUT_PATTERNS)) +def test_month_input_later_period_does_not_depend_on_earlier_requests( + system, pattern, earlier, later_period +): + """A year request caches the sum of the months; it must not carry over.""" + inputs = {"carried_monthly_flow": MONTHLY_INPUT_PATTERNS[pattern]} + fresh = _simulation(system, inputs).calculate("carried_monthly_flow", later_period) + + simulation = _simulation(system, inputs) + for period in earlier: + simulation.calculate("carried_monthly_flow", period) + + np.testing.assert_array_equal( + simulation.calculate("carried_monthly_flow", later_period), fresh + ) + + +def test_month_input_carries_latest_month_not_year_sum(system): + simulation = _simulation( + system, {"carried_monthly_flow": MONTHLY_INPUT_PATTERNS["every_month"]} + ) + np.testing.assert_array_equal( + simulation.calculate("carried_monthly_flow", BASE_YEAR), + [sum(range(1, 13)), sum(100.0 + i for i in range(12))], + ) + np.testing.assert_array_equal( + simulation.calculate("carried_monthly_flow", "2025-02"), [12.0, 111.0] + ) From 99b0ca099cc31ed6b89dfd4b0dbbab57e9172311 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 1 Oct 2026 12:58:51 -0400 Subject: [PATCH 2/6] Keep carrying over inputs stored in another unit Review of the first version found that filtering known periods to the definition period's unit also dropped authoritative inputs. A variable with no set_input helper, and every DAY variable, stores an input at whatever period it is given (a YEAR stock set for 2024-12, a MONTH stock set for 2024, a DAY stock set for 2024-12), and master carried those forward. The first version returned the default instead. The values that must not carry over are only the derived caches: calculate_divide's twelfth of a YEAR flow at a month, and calculate_add's sum of a MONTH flow at a year. STOCK paths cache nothing in another unit. So restore master's known-period handling and, in the carry-over branch, accept a period in another unit only when it was set as an input on a branch this simulation reads (_user_input_keys, filtered to visible branches, now shared with the input-export code through _get_user_input_periods). Tests: the four off-unit input cases, directly and through a branch and a nested branch. On master the 41 derived-cache cases fail; on the first version the 8 off-unit input cases fail; all 242 pass here. The docstring no longer claims the STOCK path caches month values, and names the control families. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 2 +- policyengine_core/simulations/simulation.py | 81 +++++++++------- .../core/test_carry_over_definition_period.py | 96 +++++++++++++++++-- 3 files changed, 139 insertions(+), 40 deletions(-) diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md index 7f3faba30..ceec8526a 100644 --- a/changelog.d/fix-carry-over-definition-period.fixed.md +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -1 +1 @@ -Auto-carry-over now reads only values stored at the variable's own definition period. Since 3.24.0 it could pick a month value cached for a yearly variable (a twelfth of the year's value), so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. +Auto-carry-over no longer carries a cache derived in another unit than the variable's definition period. Since 3.24.0 it could pick the twelfth of a yearly flow cached at a month by `calculate_divide` (or a monthly flow's year sum cached by `calculate_add`), so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested. In policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs stored in another unit, by variables without a `set_input` helper, still carry over. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index ae1071e07..0821132b0 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -865,22 +865,12 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # If no result, use the default value and cache it if array is None: # Check if the variable has a previously defined value - # Only a value stored at the variable's own definition period - # can stand in for another period. A YEAR variable also caches - # month values (``calculate_divide`` stores a twelfth of the - # year's value, the STOCK path the whole value) and a MONTH - # variable caches year values (``calculate_add`` stores the sum - # of the months). Those are derived from a stored value, so - # carrying one forward would rescale the variable. - known_periods = [ - known_period - for known_period in holder.get_known_periods() - if known_period.unit == variable.definition_period - ] + known_periods = holder.get_known_periods() earlier_known_periods = [ known_period for known_period in known_periods - if known_period.start < period.start + if known_period.unit == variable.definition_period + and known_period.start < period.start ] if variable.uprating is not None and len(earlier_known_periods) > 0: # Take the latest period from the filtered list itself. @@ -925,17 +915,38 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: ): # Variables with a calculate-output property specify # Sort by period.start (temporal order). Sorting Period - # tuples lexicographically compares their unit strings - # first (bug H1). Every period here has the definition - # period's unit, so a twelfth of a year's value cached at - # "2024-12" cannot stand in for "2025". - last_known_period = max(known_periods, key=lambda p: p.start) - if last_known_period.start > period.start: - return holder.default_array() - # Pass branch_name through so auto-carry-over respects the - # active branch instead of reaching for the "default" - # branch's cache (bug H2). - array = holder.get_array(last_known_period, self.branch_name) + # tuples lexicographically puts "year" before "month" + # alphabetically, so a known "2023" annual value would + # win over a later "2024-06" monthly value (bug H1). + # + # A period in a unit other than the definition period only + # counts if it was set as an input (with no ``set_input`` + # helper, an input is stored at whatever period it is + # given). Otherwise it is a derived cache: a YEAR flow + # requested for a month caches a twelfth of the year's + # value there (``calculate_divide``), and a MONTH flow + # requested for a year caches the sum of its months + # (``calculate_add``). Carrying one of those forward + # rescales the variable. + input_periods = self._get_user_input_periods(variable.name) + carry_over_periods = [ + known_period + for known_period in known_periods + if known_period.unit == variable.definition_period + or known_period in input_periods + ] + if carry_over_periods: + last_known_period = max( + carry_over_periods, key=lambda p: p.start + ) + if last_known_period.start > period.start: + return holder.default_array() + # Pass branch_name through so auto-carry-over respects + # the active branch instead of reaching for the + # "default" branch's cache (bug H2). + array = holder.get_array(last_known_period, self.branch_name) + else: + array = holder.default_array() else: array = holder.default_array() @@ -1715,6 +1726,19 @@ def _get_visible_branch_names(self) -> List[str]: branch_names.append("default") return list(dict.fromkeys(branch_names)) + def _get_user_input_periods(self, variable_name: str) -> set: + """Periods at which ``variable_name`` was set as an input on a branch + this simulation reads.""" + visible_branch_names = self._get_visible_branch_names() + return { + period + for input_variable_name, branch_name, period in getattr( + self, "_user_input_keys", set() + ) + if input_variable_name == variable_name + and branch_name in visible_branch_names + } + def _get_exportable_input_periods( self, variable_name: str, @@ -1726,14 +1750,7 @@ def _get_exportable_input_periods( if not self._is_exportable_input_variable(variable_name): return [] - user_input_periods = { - period - for input_variable_name, branch_name, period in getattr( - self, "_user_input_keys", set() - ) - if input_variable_name == variable_name - and branch_name in self._get_visible_branch_names() - } + user_input_periods = self._get_user_input_periods(variable_name) if not user_input_periods: return [] variable = self.tax_benefit_system.get_variable(variable_name) diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py index 72afa184f..fad095278 100644 --- a/tests/core/test_carry_over_definition_period.py +++ b/tests/core/test_carry_over_definition_period.py @@ -1,11 +1,12 @@ -"""Auto-carry-over reads only values stored at the variable's definition period. +"""Auto-carry-over never carries a derived cache stored in another unit. -A YEAR variable also caches month values: ``calculate_divide`` stores a -twelfth of the year's value, and the STOCK path stores the whole value. A -MONTH variable caches year values: ``calculate_add`` stores the sum of its -months. Since 3.24.0 auto-carry-over took the latest-starting known period -of any unit, so once any month of 2024 had been requested, a YEAR input -known only for 2024 carried a twelfth of its value into 2025. +A YEAR flow variable requested for a month caches a twelfth of the year's +value at that month (``calculate_divide``), and a MONTH flow variable +requested for a year caches the sum of its months (``calculate_add``). STOCK +variables cache nothing in another unit. Since 3.24.0 auto-carry-over took +the latest-starting known period of any unit, so once a month of 2024 after +January had been requested, a YEAR flow input known only for 2024 carried a +twelfth of its value into 2025. policyengine-us computes ``monthly_age`` from ``age`` one month at a time. A microsimulation that calculated 2024 and then 2025 on a single-year @@ -13,9 +14,16 @@ 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. +A period in another unit still counts when it was set as an input: a +variable with no ``set_input`` helper, and every DAY variable, stores an +input at whatever period it is given, and carry-over keeps using it. + The invariant pinned here: what a simulation calculates for a later period does not depend on which earlier periods it calculated first. Each case compares against a fresh simulation that calculates only the later period. +The YEAR flow cases fail on 3.24.0 to 3.32.11. The integer, boolean and +STOCK year cases and the MONTH cases are controls: those variables never +cached a rescaled value in another unit, so they pass on those versions too. """ from __future__ import annotations @@ -72,6 +80,40 @@ class carried_monthly_flow(Variable): label = "Monthly flow input with no formula and no uprating" +class day_stock_input(Variable): + value_type = float + entity = entities.Person + definition_period = periods.DAY + quantity_type = QuantityType.STOCK + label = "Daily stock input; DAY variables have no set_input helper" + + +class month_stock_input_without_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.MONTH + quantity_type = QuantityType.STOCK + set_input = None + label = "Monthly stock input with no set_input helper" + + +class year_stock_input_without_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + quantity_type = QuantityType.STOCK + set_input = None + label = "Yearly stock input with no set_input helper" + + +class year_flow_input_without_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + set_input = None + label = "Yearly flow input with no set_input helper" + + YEAR_INPUTS = { "carried_flow": np.array([40.0, 6.0]), "carried_stock": np.array([40.0, 6.0]), @@ -90,6 +132,10 @@ def system(): carried_count, carried_flag, carried_monthly_flow, + day_stock_input, + month_stock_input_without_helper, + year_stock_input_without_helper, + year_flow_input_without_helper, ) return system @@ -214,3 +260,39 @@ def test_month_input_carries_latest_month_not_year_sum(system): np.testing.assert_array_equal( simulation.calculate("carried_monthly_flow", "2025-02"), [12.0, 111.0] ) + + +# Inputs stored at a period in another unit than the definition period, as a +# variable with no set_input helper keeps them, and the later period asked. +OFF_UNIT_INPUTS = [ + ("day_stock_input", "2024-12", "2025-01-01"), + ("month_stock_input_without_helper", str(BASE_YEAR), "2025-01"), + ("year_stock_input_without_helper", MONTHS[-1], "2025"), + ("year_flow_input_without_helper", MONTHS[-1], "2025"), +] + + +@pytest.mark.parametrize( + "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) +) +def test_input_in_another_unit_still_carries_over( + system, variable, input_period, later_period +): + simulation = _simulation(system, {variable: {input_period: [24.0, 7.0]}}) + np.testing.assert_array_equal( + simulation.calculate(variable, later_period), [24.0, 7.0] + ) + + +@pytest.mark.parametrize( + "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) +) +def test_input_in_another_unit_set_on_a_branch_carries_over_there( + system, variable, input_period, later_period +): + simulation = _simulation(system, {}) + branch = simulation.get_branch("reform") + branch.set_input(variable, input_period, [24.0, 7.0]) + np.testing.assert_array_equal(branch.calculate(variable, later_period), [24.0, 7.0]) + nested = branch.get_branch("nested") + np.testing.assert_array_equal(nested.calculate(variable, later_period), [24.0, 7.0]) From 504e983ec97fd009a0b335963e87fdded4f3d538 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 1 Oct 2026 14:40:33 -0400 Subject: [PATCH 3/6] Mark derived caches instead of matching recorded inputs The partial re-review of 99b0ca09 found cases where a derived month cache shared its period with a recorded input and so still carried over: a same-named branch in another subtree, Simulation.clone() (which shares _user_input_keys), a parent input set after branching, an input deleted and then derived again, and derivative(). The input-key scan also costs O(input keys) per carry-over (about 19 ms at 22,000 keys). Mark the values core derives by rescaling instead: calculate_divide's twelfth of a yearly flow at a month, and calculate_add's sum over more than one sub-period. Holder.put_in_cache(..., derived=True) records the period; any later write to it (Holder._set) clears the mark; Holder.clone copies the marks. Carry-over skips marked periods and nothing else, so inputs at any unit, and off-unit values cached without the mark, carry over as on master. The uprating path is unchanged. Tests add the five leak scenarios and two that pin inputs winning over a derived cache. On master 46 of 249 cases fail, on 9e5bc21e 12, on 99b0ca09 6; all pass here. Full suite 1344 passed. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 2 +- policyengine_core/holders/holder.py | 26 ++++++- policyengine_core/simulations/simulation.py | 52 ++++++-------- .../core/test_carry_over_definition_period.py | 70 ++++++++++++++++++- 4 files changed, 115 insertions(+), 35 deletions(-) diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md index ceec8526a..d5b9a46ad 100644 --- a/changelog.d/fix-carry-over-definition-period.fixed.md +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -1 +1 @@ -Auto-carry-over no longer carries a cache derived in another unit than the variable's definition period. Since 3.24.0 it could pick the twelfth of a yearly flow cached at a month by `calculate_divide` (or a monthly flow's year sum cached by `calculate_add`), so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested. In policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs stored in another unit, by variables without a `set_input` helper, still carry over. +Auto-carry-over no longer carries a value core derived by rescaling another period's value: the twelfth of a yearly flow that `calculate_divide` caches at a month, or a sum over several sub-periods that `calculate_add` caches at the larger period. Such caches are now marked derived (`Holder.put_in_cache(..., derived=True)`) until the period is written again. Since 3.24.0 carry-over could pick a month's twelfth for a later year, so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs at any unit still carry over. diff --git a/policyengine_core/holders/holder.py b/policyengine_core/holders/holder.py index 52ae81295..fc638576f 100644 --- a/policyengine_core/holders/holder.py +++ b/policyengine_core/holders/holder.py @@ -30,6 +30,10 @@ def __init__(self, variable: "Variable", population: "Population"): is_eternal=(self.variable.definition_period == periods.ETERNITY) ) + # Periods whose stored value core derived by rescaling another + # period's value (see ``put_in_cache``); auto-carry-over skips them. + self._derived_periods = set() + # By default, do not activate on-disk storage, or variable dropping self._disk_storage = None self._on_disk_storable = False @@ -65,6 +69,7 @@ def clone(self, population: "Population") -> "Holder": new._disk_storage = ( self._disk_storage.clone() if self._disk_storage is not None else None ) + new._derived_periods = set(getattr(self, "_derived_periods", ())) new_dict["population"] = population new_dict["simulation"] = population.simulation @@ -358,14 +363,29 @@ def _set( self._disk_storage.put(value, period, branch_name) else: self._memory_storage.put(value, period, branch_name) + derived_periods = getattr(self, "_derived_periods", None) + if derived_periods and period is not None: + derived_periods.discard(periods.period(period)) if user_input_contexts: if not hasattr(simulation, "_user_input_keys"): simulation._user_input_keys = set() simulation._user_input_keys.add((self.variable.name, branch_name, period)) def put_in_cache( - self, value: ArrayLike, period: Period, branch_name: str = "default" + self, + value: ArrayLike, + period: Period, + branch_name: str = "default", + derived: bool = False, ) -> None: + """Cache ``value`` for ``period``. + + ``derived`` marks a value computed by rescaling another period's + value: a twelfth of a yearly flow cached at a month by + ``calculate_divide``, or a sum over several sub-periods cached by + ``calculate_add``. Auto-carry-over never carries such a value into + another period. Any later write to the period clears the mark. + """ if self._do_not_store: return @@ -377,6 +397,10 @@ def put_in_cache( return self._set(period, value, branch_name) + if derived: + if not hasattr(self, "_derived_periods"): + self._derived_periods = set() + self._derived_periods.add(periods.period(period)) def default_array(self) -> ArrayLike: """ diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 0821132b0..594b88aa0 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -919,21 +919,17 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # alphabetically, so a known "2023" annual value would # win over a later "2024-06" monthly value (bug H1). # - # A period in a unit other than the definition period only - # counts if it was set as an input (with no ``set_input`` - # helper, an input is stored at whatever period it is - # given). Otherwise it is a derived cache: a YEAR flow - # requested for a month caches a twelfth of the year's - # value there (``calculate_divide``), and a MONTH flow - # requested for a year caches the sum of its months - # (``calculate_add``). Carrying one of those forward - # rescales the variable. - input_periods = self._get_user_input_periods(variable.name) + # Skip values derived by rescaling another period's value: + # a YEAR flow requested for a month caches a twelfth of the + # year's value there (``calculate_divide``), and a sum over + # several sub-periods is cached at the larger period + # (``calculate_add``). Carrying either forward rescales the + # variable. Inputs, at any period, still carry over. + derived_periods = getattr(holder, "_derived_periods", ()) carry_over_periods = [ known_period for known_period in known_periods - if known_period.unit == variable.definition_period - or known_period in input_periods + if known_period not in derived_periods ] if carry_over_periods: last_known_period = max( @@ -1037,12 +1033,14 @@ def calculate_add( ) ) + sub_periods = list(period.get_subperiods(variable.definition_period)) result = sum( - self.calculate(variable_name, sub_period) - for sub_period in period.get_subperiods(variable.definition_period) + self.calculate(variable_name, sub_period) for sub_period in sub_periods ) holder = self.get_holder(variable.name) - holder.put_in_cache(result, period, self.branch_name) + holder.put_in_cache( + result, period, self.branch_name, derived=len(sub_periods) > 1 + ) return result def calculate_divide( @@ -1075,7 +1073,7 @@ def calculate_divide( computation_period = period.this_year result = self.calculate(variable_name, period=computation_period) / 12.0 holder = self.get_holder(variable.name) - holder.put_in_cache(result, period, self.branch_name) + holder.put_in_cache(result, period, self.branch_name, derived=True) return result elif period.unit == periods.YEAR: return self.calculate(variable_name, period) @@ -1726,19 +1724,6 @@ def _get_visible_branch_names(self) -> List[str]: branch_names.append("default") return list(dict.fromkeys(branch_names)) - def _get_user_input_periods(self, variable_name: str) -> set: - """Periods at which ``variable_name`` was set as an input on a branch - this simulation reads.""" - visible_branch_names = self._get_visible_branch_names() - return { - period - for input_variable_name, branch_name, period in getattr( - self, "_user_input_keys", set() - ) - if input_variable_name == variable_name - and branch_name in visible_branch_names - } - def _get_exportable_input_periods( self, variable_name: str, @@ -1750,7 +1735,14 @@ def _get_exportable_input_periods( if not self._is_exportable_input_variable(variable_name): return [] - user_input_periods = self._get_user_input_periods(variable_name) + user_input_periods = { + period + for input_variable_name, branch_name, period in getattr( + self, "_user_input_keys", set() + ) + if input_variable_name == variable_name + and branch_name in self._get_visible_branch_names() + } if not user_input_periods: return [] variable = self.tax_benefit_system.get_variable(variable_name) diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py index fad095278..df617a1a0 100644 --- a/tests/core/test_carry_over_definition_period.py +++ b/tests/core/test_carry_over_definition_period.py @@ -14,9 +14,12 @@ 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. -A period in another unit still counts when it was set as an input: a -variable with no ``set_input`` helper, and every DAY variable, stores an -input at whatever period it is given, and carry-over keeps using it. +The fix marks those two caches as derived when they are written +(``Holder.put_in_cache(..., derived=True)``), clears the mark on any later +write to the period, and never carries a marked period. Everything else +still carries over, inputs at any unit included: a variable with no +``set_input`` helper, and every DAY variable, stores an input at whatever +period it is given. The invariant pinned here: what a simulation calculates for a later period does not depend on which earlier periods it calculated first. Each case @@ -296,3 +299,64 @@ def test_input_in_another_unit_set_on_a_branch_carries_over_there( np.testing.assert_array_equal(branch.calculate(variable, later_period), [24.0, 7.0]) nested = branch.get_branch("nested") np.testing.assert_array_equal(nested.calculate(variable, later_period), [24.0, 7.0]) + + +FLOW = "year_flow_input_without_helper" +FLOW_INPUT = {FLOW: {BASE_YEAR: [120.0, 12.0]}} + + +def test_derived_cache_ignores_a_same_named_branch_elsewhere(system): + simulation = _simulation(system, FLOW_INPUT) + nested = simulation.get_branch("a").get_branch("s") + simulation.get_branch("s").set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + nested.calculate(FLOW, MONTHS[-1]) + np.testing.assert_array_equal(nested.calculate(FLOW, 2025), [120.0, 12.0]) + + +def test_derived_cache_ignores_an_input_set_on_a_clone(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate(FLOW, MONTHS[-1]) + clone = simulation.clone() + clone.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(clone.calculate(FLOW, 2025), [999.0, 999.0]) + + +def test_derived_cache_ignores_a_parent_input_set_after_branching(system): + simulation = _simulation(system, FLOW_INPUT) + branch = simulation.get_branch("b") + simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + branch.calculate(FLOW, MONTHS[-1]) + np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [999.0, 999.0]) + + +def test_derived_cache_ignores_a_deleted_input_at_its_period(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + simulation.delete_arrays(FLOW, MONTHS[-1]) + simulation.calculate(FLOW, MONTHS[-1]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + + +def test_derivative_leaves_later_years_intact(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.derivative(FLOW, FLOW, MONTHS[-1]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + + +def test_input_written_over_a_derived_cache_carries_over(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate(FLOW, MONTHS[-1]) + simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [999.0, 999.0]) + + +def test_value_cached_in_another_unit_without_the_derived_mark_carries_over(system): + simulation = _simulation(system, {}) + simulation.get_holder("day_stock_input").put_in_cache( + np.array([24.0, 7.0]), periods.period("2024-12") + ) + np.testing.assert_array_equal( + simulation.calculate("day_stock_input", "2025-01-01"), [24.0, 7.0] + ) From 886119d1e76dea25c0d485fd8f7b28cd10bdb61c Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 2 Oct 2026 01:09:13 -0400 Subject: [PATCH 4/6] Prefer own-unit periods instead of marking derived caches The third review of 504e983e found that per-period derived marks go stale or get lost on paths that write storage without Holder._set or drop entries per branch: apply_reform's input replay, dump/restore, branch-scoped Holder.delete_arrays exposing an ancestor entry, and direct storage puts. Making provenance follow every storage entry would spread across both storages, the dumper and #556's shared arrays. Use a stateless rule instead: carry over the latest period at the variable's own definition-period unit, and fall back to periods in other units only when there is none at its own unit. Derived caches (calculate_divide's twelfth at a month, calculate_add's sum at a year) always coexist with the own-unit value they came from, so they are never picked. Inputs that exist only at another unit (a variable with no set_input helper, every DAY variable) carry over as on master. Where a helper-less variable holds both an own-unit value and an input at another unit, the own-unit value now wins, as before 3.24.0; no country package has such a variable. holder.py is back to master. Tests: the lifecycle reproductions (reform replay, dump/restore, branch-scoped deletion both ways) join the leak scenarios. 253 cases; failures on master 49, 9e5bc21e 11, 99b0ca09 8, 504e983e 7, here 0. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 2 +- policyengine_core/holders/holder.py | 26 +------ policyengine_core/simulations/simulation.py | 51 ++++++------- .../core/test_carry_over_definition_period.py | 75 +++++++++++++++---- 4 files changed, 88 insertions(+), 66 deletions(-) diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md index d5b9a46ad..fb26555bf 100644 --- a/changelog.d/fix-carry-over-definition-period.fixed.md +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -1 +1 @@ -Auto-carry-over no longer carries a value core derived by rescaling another period's value: the twelfth of a yearly flow that `calculate_divide` caches at a month, or a sum over several sub-periods that `calculate_add` caches at the larger period. Such caches are now marked derived (`Holder.put_in_cache(..., derived=True)`) until the period is written again. Since 3.24.0 carry-over could pick a month's twelfth for a later year, so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs at any unit still carry over. +Auto-carry-over now prefers values stored at the variable's own definition period, and uses a period in another unit only when there is none at its own unit. Since 3.24.0 it took the latest-starting period of any unit, so it could pick the twelfth of a yearly flow that `calculate_divide` caches at a month: a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested. In policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs that exist only at another unit still carry over. diff --git a/policyengine_core/holders/holder.py b/policyengine_core/holders/holder.py index fc638576f..52ae81295 100644 --- a/policyengine_core/holders/holder.py +++ b/policyengine_core/holders/holder.py @@ -30,10 +30,6 @@ def __init__(self, variable: "Variable", population: "Population"): is_eternal=(self.variable.definition_period == periods.ETERNITY) ) - # Periods whose stored value core derived by rescaling another - # period's value (see ``put_in_cache``); auto-carry-over skips them. - self._derived_periods = set() - # By default, do not activate on-disk storage, or variable dropping self._disk_storage = None self._on_disk_storable = False @@ -69,7 +65,6 @@ def clone(self, population: "Population") -> "Holder": new._disk_storage = ( self._disk_storage.clone() if self._disk_storage is not None else None ) - new._derived_periods = set(getattr(self, "_derived_periods", ())) new_dict["population"] = population new_dict["simulation"] = population.simulation @@ -363,29 +358,14 @@ def _set( self._disk_storage.put(value, period, branch_name) else: self._memory_storage.put(value, period, branch_name) - derived_periods = getattr(self, "_derived_periods", None) - if derived_periods and period is not None: - derived_periods.discard(periods.period(period)) if user_input_contexts: if not hasattr(simulation, "_user_input_keys"): simulation._user_input_keys = set() simulation._user_input_keys.add((self.variable.name, branch_name, period)) def put_in_cache( - self, - value: ArrayLike, - period: Period, - branch_name: str = "default", - derived: bool = False, + self, value: ArrayLike, period: Period, branch_name: str = "default" ) -> None: - """Cache ``value`` for ``period``. - - ``derived`` marks a value computed by rescaling another period's - value: a twelfth of a yearly flow cached at a month by - ``calculate_divide``, or a sum over several sub-periods cached by - ``calculate_add``. Auto-carry-over never carries such a value into - another period. Any later write to the period clears the mark. - """ if self._do_not_store: return @@ -397,10 +377,6 @@ def put_in_cache( return self._set(period, value, branch_name) - if derived: - if not hasattr(self, "_derived_periods"): - self._derived_periods = set() - self._derived_periods.add(periods.period(period)) def default_array(self) -> ArrayLike: """ diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 594b88aa0..22cdd8753 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -919,30 +919,29 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # alphabetically, so a known "2023" annual value would # win over a later "2024-06" monthly value (bug H1). # - # Skip values derived by rescaling another period's value: - # a YEAR flow requested for a month caches a twelfth of the - # year's value there (``calculate_divide``), and a sum over - # several sub-periods is cached at the larger period - # (``calculate_add``). Carrying either forward rescales the - # variable. Inputs, at any period, still carry over. - derived_periods = getattr(holder, "_derived_periods", ()) - carry_over_periods = [ + # Prefer values at the variable's own definition period. A + # YEAR flow requested for a month caches a twelfth of the + # year's value at that month (``calculate_divide``), and a + # MONTH flow requested for a year caches the sum of its + # months (``calculate_add``): both coexist with the value + # they were derived from, and carrying either forward + # rescales the variable. A value at another unit is used + # only when there is none at the variable's own unit, as for + # an input to a variable with no ``set_input`` helper. + own_unit_periods = [ known_period for known_period in known_periods - if known_period not in derived_periods + if known_period.unit == variable.definition_period ] - if carry_over_periods: - last_known_period = max( - carry_over_periods, key=lambda p: p.start - ) - if last_known_period.start > period.start: - return holder.default_array() - # Pass branch_name through so auto-carry-over respects - # the active branch instead of reaching for the - # "default" branch's cache (bug H2). - array = holder.get_array(last_known_period, self.branch_name) - else: - array = holder.default_array() + last_known_period = max( + own_unit_periods or known_periods, key=lambda p: p.start + ) + if last_known_period.start > period.start: + return holder.default_array() + # Pass branch_name through so auto-carry-over respects the + # active branch instead of reaching for the "default" + # branch's cache (bug H2). + array = holder.get_array(last_known_period, self.branch_name) else: array = holder.default_array() @@ -1033,14 +1032,12 @@ def calculate_add( ) ) - sub_periods = list(period.get_subperiods(variable.definition_period)) result = sum( - self.calculate(variable_name, sub_period) for sub_period in sub_periods + self.calculate(variable_name, sub_period) + for sub_period in period.get_subperiods(variable.definition_period) ) holder = self.get_holder(variable.name) - holder.put_in_cache( - result, period, self.branch_name, derived=len(sub_periods) > 1 - ) + holder.put_in_cache(result, period, self.branch_name) return result def calculate_divide( @@ -1073,7 +1070,7 @@ def calculate_divide( computation_period = period.this_year result = self.calculate(variable_name, period=computation_period) / 12.0 holder = self.get_holder(variable.name) - holder.put_in_cache(result, period, self.branch_name, derived=True) + holder.put_in_cache(result, period, self.branch_name) return result elif period.unit == periods.YEAR: return self.calculate(variable_name, period) diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py index df617a1a0..366b1a15b 100644 --- a/tests/core/test_carry_over_definition_period.py +++ b/tests/core/test_carry_over_definition_period.py @@ -1,4 +1,4 @@ -"""Auto-carry-over never carries a derived cache stored in another unit. +"""Auto-carry-over prefers values at the variable's own definition period. A YEAR flow variable requested for a month caches a twelfth of the year's value at that month (``calculate_divide``), and a MONTH flow variable @@ -14,12 +14,14 @@ 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. -The fix marks those two caches as derived when they are written -(``Holder.put_in_cache(..., derived=True)``), clears the mark on any later -write to the period, and never carries a marked period. Everything else -still carries over, inputs at any unit included: a variable with no -``set_input`` helper, and every DAY variable, stores an input at whatever -period it is given. +Both caches coexist with the value they were derived from, at the +variable's own unit, so carry-over now picks the latest own-unit period and +uses another unit only when the variable has nothing at its own unit. That +keeps inputs that exist only at another unit carrying over: a variable with +no ``set_input`` helper, and every DAY variable, stores an input at whatever +period it is given. Where such a variable holds both a value at its own unit +and an input at another unit, the own-unit value now wins, as it did before +3.24.0. The invariant pinned here: what a simulation calculates for a later period does not depend on which earlier periods it calculated first. Each case @@ -319,7 +321,8 @@ def test_derived_cache_ignores_an_input_set_on_a_clone(system): clone = simulation.clone() clone.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) - np.testing.assert_array_equal(clone.calculate(FLOW, 2025), [999.0, 999.0]) + # The clone's year value wins over its month input. + np.testing.assert_array_equal(clone.calculate(FLOW, 2025), [120.0, 12.0]) def test_derived_cache_ignores_a_parent_input_set_after_branching(system): @@ -328,7 +331,7 @@ def test_derived_cache_ignores_a_parent_input_set_after_branching(system): simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) branch.calculate(FLOW, MONTHS[-1]) np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [999.0, 999.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) def test_derived_cache_ignores_a_deleted_input_at_its_period(system): @@ -345,14 +348,15 @@ def test_derivative_leaves_later_years_intact(system): np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) -def test_input_written_over_a_derived_cache_carries_over(system): +def test_own_unit_value_wins_over_an_input_at_another_unit(system): + """A change from 3.24.0-3.32.11, which carried the later-starting month + input; earlier versions carried the year value, as this does.""" simulation = _simulation(system, FLOW_INPUT) - simulation.calculate(FLOW, MONTHS[-1]) simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [999.0, 999.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) -def test_value_cached_in_another_unit_without_the_derived_mark_carries_over(system): +def test_value_cached_only_at_another_unit_carries_over(system): simulation = _simulation(system, {}) simulation.get_holder("day_stock_input").put_in_cache( np.array([24.0, 7.0]), periods.period("2024-12") @@ -360,3 +364,48 @@ def test_value_cached_in_another_unit_without_the_derived_mark_carries_over(syst np.testing.assert_array_equal( simulation.calculate("day_stock_input", "2025-01-01"), [24.0, 7.0] ) + + +# Lifecycle paths: nothing is tracked per period, so writes, deletions, +# replays and persistence cannot leave a rule stale. + + +def test_reform_replay_restores_an_input_at_another_unit(system): + simulation = _simulation(system, {FLOW: {"2024-06": [600.0, 60.0]}}) + branch = simulation.get_branch("child") + branch.calculate_divide(FLOW, "2024-06") + branch.apply_reform({}) + np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [600.0, 60.0]) + + +def test_dump_and_restore_keep_carrying_the_year_value(system, tmp_path): + from policyengine_core.tools.simulation_dumper import ( + dump_simulation, + restore_simulation, + ) + + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate_divide(FLOW, "2024-06") + dump_simulation(simulation, str(tmp_path / "dump")) + restored = restore_simulation(str(tmp_path / "dump"), system) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(restored.calculate(FLOW, 2025), [120.0, 12.0]) + + +def test_deleting_a_branch_cache_exposes_the_ancestor_input(system): + simulation = _simulation(system, {FLOW: {"2024-06": [600.0, 60.0]}}) + branch = simulation.get_branch("child") + branch.calculate_divide(FLOW, "2024-06") + branch.get_holder(FLOW).delete_arrays("2024-06", "child") + np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [600.0, 60.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [600.0, 60.0]) + + +def test_deleting_a_branch_input_does_not_expose_the_ancestor_twelfth(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate_divide(FLOW, "2024-06") + branch = simulation.get_branch("child") + branch.set_input(FLOW, "2024-06", [600.0, 60.0]) + branch.get_holder(FLOW).delete_arrays("2024-06", "child") + np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) From 7af22b16ff42c2246249aa13d86d300a5ddfae65 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 2 Oct 2026 02:35:24 -0400 Subject: [PATCH 5/6] Ignore other-unit periods only for variables with a set_input helper The fourth review of bab4325a found that preferring own-unit periods for every variable made variables without a set_input helper (DAY variables, set_input = None) depend on request order where master did not: a default cached for 2025, or a calculate_add sum stored for 2024, now beat a later input at another unit. A set_input helper stores every input at the definition period (Holder.set_input routes other periods to it; both helpers store at the definition unit or raise), so for a helper variable a value at another unit is never an input: it is calculate_divide's twelfth, calculate_add's sum, or an explicit put_in_cache. Drop those from the known periods for helper variables, at the start of the no-formula branch; master's carry-over code is otherwise unchanged. Variables without a helper keep master's rule (latest period of any unit). No variable in policyengine-us, -uk or -canada lacks a helper. Tests rewritten around that split (235 cases): helper variables pin order independence, the leak and lifecycle scenarios, no backward carry-over, and never carrying another unit; helper-less variables pin master's behaviour, including the review's four order cases and the known limitation that they still carry a cached twelfth. The 17 tautological empty-request cases are gone. Master fails 48 (all helper-variable fix cases) and passes every helper-less case; bab4325a fails 4; mutants (no early return, min for max, filter all, filter none, inverted) are killed. Docstring, code comment and changelog no longer overstate. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 2 +- policyengine_core/simulations/simulation.py | 37 +-- .../core/test_carry_over_definition_period.py | 294 ++++++++++-------- 3 files changed, 187 insertions(+), 146 deletions(-) diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md index fb26555bf..80054132f 100644 --- a/changelog.d/fix-carry-over-definition-period.fixed.md +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -1 +1 @@ -Auto-carry-over now prefers values stored at the variable's own definition period, and uses a period in another unit only when there is none at its own unit. Since 3.24.0 it took the latest-starting period of any unit, so it could pick the twelfth of a yearly flow that `calculate_divide` caches at a month: a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested. In policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Inputs that exist only at another unit still carry over. +Auto-carry-over no longer carries a value held at a unit other than the variable's definition period when the variable has a `set_input` helper. A helper stores every input at the definition period, so such values are caches: the twelfth of a yearly flow that `calculate_divide` caches at a month, or a sum that `calculate_add` caches at a larger period. Since 3.24.0 carry-over took the latest-starting period of any unit, so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Variables without a helper (DAY variables, and any declaring `set_input = None`) are unchanged. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index d767608cd..e6ffda48b 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -895,6 +895,24 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: if array is None: # Check if the variable has a previously defined value known_periods = holder.get_known_periods() + if variable.set_input is not None: + # A ``set_input`` helper stores every input at the + # variable's definition period (it converts an input given + # for another period), so for such a variable a value at + # another unit is never an input. The ones core writes are + # derived from a stored value: a YEAR flow requested for a + # month caches a twelfth of the year's value there + # (``calculate_divide``), and a sum over several + # sub-periods is cached at the larger period + # (``calculate_add``). Carrying either forward rescales the + # variable. Variables without a helper (DAY variables, and + # any declaring ``set_input = None``) store inputs at + # whatever period they are given and keep every period. + known_periods = [ + known_period + for known_period in known_periods + if known_period.unit == variable.definition_period + ] earlier_known_periods = [ known_period for known_period in known_periods @@ -947,24 +965,7 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # tuples lexicographically puts "year" before "month" # alphabetically, so a known "2023" annual value would # win over a later "2024-06" monthly value (bug H1). - # - # Prefer values at the variable's own definition period. A - # YEAR flow requested for a month caches a twelfth of the - # year's value at that month (``calculate_divide``), and a - # MONTH flow requested for a year caches the sum of its - # months (``calculate_add``): both coexist with the value - # they were derived from, and carrying either forward - # rescales the variable. A value at another unit is used - # only when there is none at the variable's own unit, as for - # an input to a variable with no ``set_input`` helper. - own_unit_periods = [ - known_period - for known_period in known_periods - if known_period.unit == variable.definition_period - ] - last_known_period = max( - own_unit_periods or known_periods, key=lambda p: p.start - ) + last_known_period = max(known_periods, key=lambda p: p.start) if last_known_period.start > period.start: return holder.default_array() # Pass branch_name through so auto-carry-over respects the diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py index 366b1a15b..71ce810fc 100644 --- a/tests/core/test_carry_over_definition_period.py +++ b/tests/core/test_carry_over_definition_period.py @@ -1,12 +1,11 @@ -"""Auto-carry-over prefers values at the variable's own definition period. +"""Auto-carry-over and values cached at a unit other than the definition period. A YEAR flow variable requested for a month caches a twelfth of the year's -value at that month (``calculate_divide``), and a MONTH flow variable -requested for a year caches the sum of its months (``calculate_add``). STOCK -variables cache nothing in another unit. Since 3.24.0 auto-carry-over took -the latest-starting known period of any unit, so once a month of 2024 after -January had been requested, a YEAR flow input known only for 2024 carried a -twelfth of its value into 2025. +value at that month (``calculate_divide``), and a sum over several +sub-periods is cached at the larger period (``calculate_add``). Since 3.24.0 +auto-carry-over took the latest-starting known period of any unit, so once a +month of 2024 after January had been requested, a YEAR flow input known only +for 2024 carried a twelfth of its value into 2025. policyengine-us computes ``monthly_age`` from ``age`` one month at a time. A microsimulation that calculated 2024 and then 2025 on a single-year @@ -14,21 +13,20 @@ 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. -Both caches coexist with the value they were derived from, at the -variable's own unit, so carry-over now picks the latest own-unit period and -uses another unit only when the variable has nothing at its own unit. That -keeps inputs that exist only at another unit carrying over: a variable with -no ``set_input`` helper, and every DAY variable, stores an input at whatever -period it is given. Where such a variable holds both a value at its own unit -and an input at another unit, the own-unit value now wins, as it did before -3.24.0. - -The invariant pinned here: what a simulation calculates for a later period -does not depend on which earlier periods it calculated first. Each case -compares against a fresh simulation that calculates only the later period. -The YEAR flow cases fail on 3.24.0 to 3.32.11. The integer, boolean and -STOCK year cases and the MONTH cases are controls: those variables never -cached a rescaled value in another unit, so they pass on those versions too. +A variable with a ``set_input`` helper stores every input at its definition +period, so a value it holds at another unit is never an input; carry-over now +ignores those for such variables. Variables without a helper (DAY variables, +and any declaring ``set_input = None``) store inputs at whatever period they +are given, so core cannot tell an input from a derived cache there; they keep +the rule of 3.24.0 to 3.32.x, the latest period of any unit. No variable in +policyengine-us, policyengine-uk or policyengine-canada lacks a helper. + +The invariant pinned for helper variables: what a simulation calculates for +a later period does not depend on which earlier periods it calculated first. +Each such case compares against a fresh simulation that calculates only the +later period. The YEAR flow cases fail on 3.24.0 to 3.32.x; the integer, +boolean and STOCK year cases and the MONTH cases are controls that pass there +too. """ from __future__ import annotations @@ -49,6 +47,9 @@ MONTHS = [f"{BASE_YEAR}-{month:02d}" for month in range(1, 13)] +# Variables with a set_input helper (the default for YEAR and MONTH). + + class carried_flow(Variable): value_type = float entity = entities.Person @@ -85,6 +86,9 @@ class carried_monthly_flow(Variable): label = "Monthly flow input with no formula and no uprating" +# Variables without a helper. + + class day_stock_input(Variable): value_type = float entity = entities.Person @@ -125,6 +129,9 @@ class year_flow_input_without_helper(Variable): "carried_count": np.array([40, 6]), "carried_flag": np.array([True, False]), } +FLOW = "carried_flow" +FLOW_INPUT = {FLOW: {BASE_YEAR: [120.0, 12.0]}} +NO_HELPER_FLOW = "year_flow_input_without_helper" @pytest.fixture(scope="module") @@ -158,7 +165,6 @@ def _simulation(system, inputs): # Earlier requests that leave month (or year) caches behind before a later # period is calculated. EARLIER_REQUESTS = [ - [], [str(BASE_YEAR)], [MONTHS[0]], [MONTHS[5]], @@ -170,22 +176,36 @@ def _simulation(system, inputs): LATER_PERIODS = ["2025", "2027", "2025-03", "2027-12"] +def _ids(requests): + return "+".join(requests) + + +# Variables with a helper: never carry a value at another unit. + + def test_month_cache_of_year_input_does_not_carry_into_next_year(system): """The policyengine-us ``monthly_age`` sequence, on a synthetic variable.""" - simulation = _simulation(system, {"carried_flow": {BASE_YEAR: [40.0, 6.0]}}) + simulation = _simulation(system, {FLOW: {BASE_YEAR: [40.0, 6.0]}}) - december = simulation.calculate("carried_flow", MONTHS[-1]) + december = simulation.calculate(FLOW, MONTHS[-1]) np.testing.assert_allclose(december, [40.0 / 12, 6.0 / 12]) np.testing.assert_array_equal( - simulation.calculate("carried_flow", BASE_YEAR + 1), [40.0, 6.0] + simulation.calculate(FLOW, BASE_YEAR + 1), [40.0, 6.0] + ) + + +@pytest.mark.parametrize("later_year", [2025, 2027]) +@pytest.mark.parametrize("variable", sorted(YEAR_INPUTS)) +def test_year_input_carries_over_unchanged(system, variable, later_year): + simulation = _simulation(system, {variable: {BASE_YEAR: YEAR_INPUTS[variable]}}) + np.testing.assert_array_equal( + simulation.calculate(variable, later_year), YEAR_INPUTS[variable] ) @pytest.mark.parametrize("later_period", LATER_PERIODS) -@pytest.mark.parametrize( - "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" -) +@pytest.mark.parametrize("earlier", EARLIER_REQUESTS, ids=_ids) @pytest.mark.parametrize("variable", sorted(YEAR_INPUTS)) def test_year_input_later_period_does_not_depend_on_earlier_requests( system, variable, earlier, later_period @@ -200,30 +220,25 @@ def test_year_input_later_period_does_not_depend_on_earlier_requests( np.testing.assert_array_equal(result, fresh) if periods.period(later_period).unit == periods.YEAR: - # Carry-over of an input is the input itself. np.testing.assert_array_equal(result, YEAR_INPUTS[variable]) @pytest.mark.parametrize("later_period", LATER_PERIODS) -@pytest.mark.parametrize( - "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" -) +@pytest.mark.parametrize("earlier", EARLIER_REQUESTS, ids=_ids) def test_year_input_branch_does_not_depend_on_earlier_requests( system, earlier, later_period ): """A branch forked after the earlier requests clones their month caches.""" - inputs = {"carried_flow": {BASE_YEAR: YEAR_INPUTS["carried_flow"]}} - fresh = _simulation(system, inputs).calculate("carried_flow", later_period) + inputs = {FLOW: {BASE_YEAR: YEAR_INPUTS[FLOW]}} + fresh = _simulation(system, inputs).calculate(FLOW, later_period) simulation = _simulation(system, inputs) for period in earlier: - simulation.calculate("carried_flow", period) + simulation.calculate(FLOW, period) branch = simulation.get_branch("itemizing") - np.testing.assert_array_equal(branch.calculate("carried_flow", later_period), fresh) - np.testing.assert_array_equal( - simulation.calculate("carried_flow", later_period), fresh - ) + np.testing.assert_array_equal(branch.calculate(FLOW, later_period), fresh) + np.testing.assert_array_equal(simulation.calculate(FLOW, later_period), fresh) MONTHLY_INPUT_PATTERNS = { @@ -234,9 +249,7 @@ def test_year_input_branch_does_not_depend_on_earlier_requests( @pytest.mark.parametrize("later_period", ["2025-01", "2025-07", "2026"]) -@pytest.mark.parametrize( - "earlier", EARLIER_REQUESTS, ids=lambda e: "+".join(e) or "none" -) +@pytest.mark.parametrize("earlier", EARLIER_REQUESTS, ids=_ids) @pytest.mark.parametrize("pattern", sorted(MONTHLY_INPUT_PATTERNS)) def test_month_input_later_period_does_not_depend_on_earlier_requests( system, pattern, earlier, later_period @@ -267,44 +280,25 @@ def test_month_input_carries_latest_month_not_year_sum(system): ) -# Inputs stored at a period in another unit than the definition period, as a -# variable with no set_input helper keeps them, and the later period asked. -OFF_UNIT_INPUTS = [ - ("day_stock_input", "2024-12", "2025-01-01"), - ("month_stock_input_without_helper", str(BASE_YEAR), "2025-01"), - ("year_stock_input_without_helper", MONTHS[-1], "2025"), - ("year_flow_input_without_helper", MONTHS[-1], "2025"), -] +def test_no_carry_over_backwards_from_a_later_input(system): + simulation = _simulation(system, {FLOW: {2026: [120.0, 12.0]}}) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [0.0, 0.0]) -@pytest.mark.parametrize( - "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) -) -def test_input_in_another_unit_still_carries_over( - system, variable, input_period, later_period -): - simulation = _simulation(system, {variable: {input_period: [24.0, 7.0]}}) - np.testing.assert_array_equal( - simulation.calculate(variable, later_period), [24.0, 7.0] - ) - - -@pytest.mark.parametrize( - "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) -) -def test_input_in_another_unit_set_on_a_branch_carries_over_there( - system, variable, input_period, later_period -): +def test_variable_with_helper_never_carries_another_unit(system): + """Even when the value at another unit is all it holds.""" simulation = _simulation(system, {}) - branch = simulation.get_branch("reform") - branch.set_input(variable, input_period, [24.0, 7.0]) - np.testing.assert_array_equal(branch.calculate(variable, later_period), [24.0, 7.0]) - nested = branch.get_branch("nested") - np.testing.assert_array_equal(nested.calculate(variable, later_period), [24.0, 7.0]) + simulation.get_holder(FLOW).put_in_cache( + np.array([10.0, 1.0]), periods.period(MONTHS[-1]) + ) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [0.0, 0.0]) -FLOW = "year_flow_input_without_helper" -FLOW_INPUT = {FLOW: {BASE_YEAR: [120.0, 12.0]}} +def test_branch_never_carries_an_inherited_twelfth(system): + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate(FLOW, "2024-06") + branch = simulation.get_branch("child") + np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) def test_derived_cache_ignores_a_same_named_branch_elsewhere(system): @@ -319,10 +313,11 @@ def test_derived_cache_ignores_an_input_set_on_a_clone(system): simulation = _simulation(system, FLOW_INPUT) simulation.calculate(FLOW, MONTHS[-1]) clone = simulation.clone() + # The helper stores a month input for a yearly variable as the year + # starting that month, which then carries over in the clone. clone.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) - # The clone's year value wins over its month input. - np.testing.assert_array_equal(clone.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(clone.calculate(FLOW, 2025), [999.0, 999.0]) def test_derived_cache_ignores_a_parent_input_set_after_branching(system): @@ -331,15 +326,7 @@ def test_derived_cache_ignores_a_parent_input_set_after_branching(system): simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) branch.calculate(FLOW, MONTHS[-1]) np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) - - -def test_derived_cache_ignores_a_deleted_input_at_its_period(system): - simulation = _simulation(system, FLOW_INPUT) - simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) - simulation.delete_arrays(FLOW, MONTHS[-1]) - simulation.calculate(FLOW, MONTHS[-1]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [999.0, 999.0]) def test_derivative_leaves_later_years_intact(system): @@ -348,64 +335,117 @@ def test_derivative_leaves_later_years_intact(system): np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) -def test_own_unit_value_wins_over_an_input_at_another_unit(system): - """A change from 3.24.0-3.32.11, which carried the later-starting month - input; earlier versions carried the year value, as this does.""" +def test_dump_and_restore_keep_carrying_the_year_value(system, tmp_path): + from policyengine_core.tools.simulation_dumper import ( + dump_simulation, + restore_simulation, + ) + simulation = _simulation(system, FLOW_INPUT) - simulation.set_input(FLOW, MONTHS[-1], [999.0, 999.0]) + simulation.calculate_divide(FLOW, "2024-06") + dump_simulation(simulation, str(tmp_path / "dump")) + restored = restore_simulation(str(tmp_path / "dump"), system) np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + np.testing.assert_array_equal(restored.calculate(FLOW, 2025), [120.0, 12.0]) + + +# Variables without a helper keep the rule of 3.24.0 to 3.32.x: the latest +# known period of any unit. + +OFF_UNIT_INPUTS = [ + ("day_stock_input", "2024-12", "2025-01-01"), + ("month_stock_input_without_helper", str(BASE_YEAR), "2025-01"), + ("year_stock_input_without_helper", MONTHS[-1], "2025"), + (NO_HELPER_FLOW, MONTHS[-1], "2025"), +] -def test_value_cached_only_at_another_unit_carries_over(system): +@pytest.mark.parametrize( + "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) +) +def test_input_in_another_unit_carries_over( + system, variable, input_period, later_period +): + simulation = _simulation(system, {variable: {input_period: [24.0, 7.0]}}) + np.testing.assert_array_equal( + simulation.calculate(variable, later_period), [24.0, 7.0] + ) + + +@pytest.mark.parametrize( + "variable,input_period,later_period", OFF_UNIT_INPUTS, ids=lambda x: str(x) +) +def test_input_in_another_unit_set_on_a_branch_carries_over_there( + system, variable, input_period, later_period +): simulation = _simulation(system, {}) - simulation.get_holder("day_stock_input").put_in_cache( - np.array([24.0, 7.0]), periods.period("2024-12") + branch = simulation.get_branch("reform") + branch.set_input(variable, input_period, [24.0, 7.0]) + np.testing.assert_array_equal(branch.calculate(variable, later_period), [24.0, 7.0]) + nested = branch.get_branch("nested") + np.testing.assert_array_equal(nested.calculate(variable, later_period), [24.0, 7.0]) + + +def test_latest_of_several_inputs_in_another_unit_carries_over(system): + simulation = _simulation( + system, {NO_HELPER_FLOW: {"2024-03": [3.0, 3.0], "2024-09": [9.0, 9.0]}} ) np.testing.assert_array_equal( - simulation.calculate("day_stock_input", "2025-01-01"), [24.0, 7.0] + simulation.calculate(NO_HELPER_FLOW, 2025), [9.0, 9.0] ) -# Lifecycle paths: nothing is tracked per period, so writes, deletions, -# replays and persistence cannot leave a rule stale. +def test_later_input_in_another_unit_beats_an_earlier_cached_default(system): + """A default cached for 2025 does not shadow a later month input.""" + inputs = {NO_HELPER_FLOW: {"2025-09": [68.0, 6.0]}} + fresh = _simulation(system, inputs).calculate(NO_HELPER_FLOW, 2026) + simulation = _simulation(system, {}) + simulation.calculate(NO_HELPER_FLOW, 2025) + simulation.set_input(NO_HELPER_FLOW, "2025-09", [68.0, 6.0]) + result = simulation.calculate(NO_HELPER_FLOW, 2026) -def test_reform_replay_restores_an_input_at_another_unit(system): - simulation = _simulation(system, {FLOW: {"2024-06": [600.0, 60.0]}}) - branch = simulation.get_branch("child") - branch.calculate_divide(FLOW, "2024-06") - branch.apply_reform({}) - np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [600.0, 60.0]) + np.testing.assert_array_equal(fresh, [68.0, 6.0]) + np.testing.assert_array_equal(result, fresh) -def test_dump_and_restore_keep_carrying_the_year_value(system, tmp_path): - from policyengine_core.tools.simulation_dumper import ( - dump_simulation, - restore_simulation, - ) +def test_input_in_another_unit_beats_an_earlier_year_sum(system): + inputs = {NO_HELPER_FLOW: {"2025-03": [23.0, 2.0]}} + fresh = _simulation(system, inputs).calculate(NO_HELPER_FLOW, 2026) - simulation = _simulation(system, FLOW_INPUT) - simulation.calculate_divide(FLOW, "2024-06") - dump_simulation(simulation, str(tmp_path / "dump")) - restored = restore_simulation(str(tmp_path / "dump"), system) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) - np.testing.assert_array_equal(restored.calculate(FLOW, 2025), [120.0, 12.0]) + simulation = _simulation(system, inputs) + simulation.calculate_add(NO_HELPER_FLOW, BASE_YEAR) + result = simulation.calculate(NO_HELPER_FLOW, 2026) + np.testing.assert_array_equal(fresh, [23.0, 2.0]) + np.testing.assert_array_equal(result, fresh) -def test_deleting_a_branch_cache_exposes_the_ancestor_input(system): - simulation = _simulation(system, {FLOW: {"2024-06": [600.0, 60.0]}}) + +def test_reform_replay_restores_an_input_at_another_unit(system): + simulation = _simulation(system, {NO_HELPER_FLOW: {"2024-06": [600.0, 60.0]}}) branch = simulation.get_branch("child") - branch.calculate_divide(FLOW, "2024-06") - branch.get_holder(FLOW).delete_arrays("2024-06", "child") - np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [600.0, 60.0]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [600.0, 60.0]) + branch.calculate_divide(NO_HELPER_FLOW, "2024-06") + branch.apply_reform({}) + np.testing.assert_array_equal(branch.calculate(NO_HELPER_FLOW, 2025), [600.0, 60.0]) -def test_deleting_a_branch_input_does_not_expose_the_ancestor_twelfth(system): - simulation = _simulation(system, FLOW_INPUT) - simulation.calculate_divide(FLOW, "2024-06") +def test_deleting_a_branch_cache_exposes_the_ancestor_input(system): + simulation = _simulation(system, {NO_HELPER_FLOW: {"2024-06": [600.0, 60.0]}}) branch = simulation.get_branch("child") - branch.set_input(FLOW, "2024-06", [600.0, 60.0]) - branch.get_holder(FLOW).delete_arrays("2024-06", "child") - np.testing.assert_array_equal(branch.calculate(FLOW, 2025), [120.0, 12.0]) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + branch.calculate_divide(NO_HELPER_FLOW, "2024-06") + branch.get_holder(NO_HELPER_FLOW).delete_arrays("2024-06", "child") + np.testing.assert_array_equal(branch.calculate(NO_HELPER_FLOW, 2025), [600.0, 60.0]) + np.testing.assert_array_equal( + simulation.calculate(NO_HELPER_FLOW, 2025), [600.0, 60.0] + ) + + +def test_variable_without_helper_still_carries_a_derived_twelfth(system): + """A known limitation, unchanged here: such a variable can hold an input at + any unit, so carry-over cannot tell a cached twelfth from an input and + takes the latest period. No country package has such a variable.""" + simulation = _simulation(system, {NO_HELPER_FLOW: {BASE_YEAR: [120.0, 12.0]}}) + simulation.calculate(NO_HELPER_FLOW, MONTHS[-1]) + np.testing.assert_array_equal( + simulation.calculate(NO_HELPER_FLOW, 2025), [10.0, 1.0] + ) From 97d4582a907bd1912ca03be9c6698d86ab5d4d0e Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 2 Oct 2026 05:16:04 -0400 Subject: [PATCH 6/6] Prefer own-unit periods for helper variables, falling back to all The fifth review of 7af22b16 found that dropping other-unit periods for helper variables loses genuine inputs on supported paths: a reform that changes a variable's definition period (YEAR<->MONTH) or installs a helper after an off-unit input, an automatic reform baseline, a restore into a system with other definitions, and custom helpers. In each the off-unit input is the variable's only data, and master carries it. Prefer own-unit periods for a variable with a (truthy) set_input helper and fall back to every period when there are none, in the carry-over branch only; master's code is otherwise unchanged. The fifth review's 32-case premise harness now differs from master in one case, restore_builtin_same_system, where master carries the derived twelfth (10) and this carries the year input (120). Tests (244): the reform, helper-installation and cross-system restore paths keep their input; a helper variable with nothing at its own unit uses another unit, as before; set_input = False counts as no helper; mutant killers for FLOW-only, divide-helper-only, early return on unfiltered periods, no fallback and no gating. The known limit (a helper variable with both units carries its own unit) is pinned. Failures: master 51, bab4325a 4, 7af22b16 6, here 0; every listed mutant fails. Co-Authored-By: Claude Opus 5.5 --- .../fix-carry-over-definition-period.fixed.md | 2 +- policyengine_core/simulations/simulation.py | 47 ++-- .../core/test_carry_over_definition_period.py | 235 +++++++++++++++--- 3 files changed, 234 insertions(+), 50 deletions(-) diff --git a/changelog.d/fix-carry-over-definition-period.fixed.md b/changelog.d/fix-carry-over-definition-period.fixed.md index 80054132f..b4a6ea849 100644 --- a/changelog.d/fix-carry-over-definition-period.fixed.md +++ b/changelog.d/fix-carry-over-definition-period.fixed.md @@ -1 +1 @@ -Auto-carry-over no longer carries a value held at a unit other than the variable's definition period when the variable has a `set_input` helper. A helper stores every input at the definition period, so such values are caches: the twelfth of a yearly flow that `calculate_divide` caches at a month, or a sum that `calculate_add` caches at a larger period. Since 3.24.0 carry-over took the latest-starting period of any unit, so a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested; in policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Variables without a helper (DAY variables, and any declaring `set_input = None`) are unchanged. +Auto-carry-over now prefers, for a variable with a `set_input` helper, periods at the variable's definition unit, and uses other units only when it has nothing there. Since 3.24.0 it took the latest-starting period of any unit, so it could pick the twelfth of a yearly flow that `calculate_divide` caches at a month: a yearly input known only for one year came out at a twelfth of its value in later years once any month after January had been requested. In policyengine-us this set every person's age to a twelfth of its value in the second year of a multi-year simulation. Variables without a helper (DAY variables, `set_input = None`) are unchanged. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index e6ffda48b..fbdf13cce 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -895,24 +895,6 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: if array is None: # Check if the variable has a previously defined value known_periods = holder.get_known_periods() - if variable.set_input is not None: - # A ``set_input`` helper stores every input at the - # variable's definition period (it converts an input given - # for another period), so for such a variable a value at - # another unit is never an input. The ones core writes are - # derived from a stored value: a YEAR flow requested for a - # month caches a twelfth of the year's value there - # (``calculate_divide``), and a sum over several - # sub-periods is cached at the larger period - # (``calculate_add``). Carrying either forward rescales the - # variable. Variables without a helper (DAY variables, and - # any declaring ``set_input = None``) store inputs at - # whatever period they are given and keep every period. - known_periods = [ - known_period - for known_period in known_periods - if known_period.unit == variable.definition_period - ] earlier_known_periods = [ known_period for known_period in known_periods @@ -965,7 +947,34 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: # tuples lexicographically puts "year" before "month" # alphabetically, so a known "2023" annual value would # win over a later "2024-06" monthly value (bug H1). - last_known_period = max(known_periods, key=lambda p: p.start) + # + # A variable with a ``set_input`` helper prefers periods at + # its definition unit. The helper stores inputs at the + # definition period, so a value at another unit beside them + # is ordinarily a cache derived from them: a YEAR flow + # requested for a month caches a twelfth of the year's value + # there (``calculate_divide``), and a sum over several + # sub-periods is cached at the larger period + # (``calculate_add``). Carrying either forward rescales the + # variable. A value at another unit can still be a genuine + # input (stored before a reform changed the variable's + # definition period or helper, restored from a simulation + # with other definitions, or written by a custom helper), so + # other units are used when there is nothing at the + # definition unit. Variables without a helper (DAY + # variables, ``set_input = None``) keep every period. + own_unit_periods = ( + [ + known_period + for known_period in known_periods + if known_period.unit == variable.definition_period + ] + if variable.set_input + else [] + ) + last_known_period = max( + own_unit_periods or known_periods, key=lambda p: p.start + ) if last_known_period.start > period.start: return holder.default_array() # Pass branch_name through so auto-carry-over respects the diff --git a/tests/core/test_carry_over_definition_period.py b/tests/core/test_carry_over_definition_period.py index 71ce810fc..e871580b9 100644 --- a/tests/core/test_carry_over_definition_period.py +++ b/tests/core/test_carry_over_definition_period.py @@ -13,20 +13,28 @@ 3,000-household Enhanced CPS subsample, 2025 federal income tax came to $31.6bn, against $1,846.9bn in a simulation that calculated only 2025. -A variable with a ``set_input`` helper stores every input at its definition -period, so a value it holds at another unit is never an input; carry-over now -ignores those for such variables. Variables without a helper (DAY variables, -and any declaring ``set_input = None``) store inputs at whatever period they -are given, so core cannot tell an input from a derived cache there; they keep -the rule of 3.24.0 to 3.32.x, the latest period of any unit. No variable in -policyengine-us, policyengine-uk or policyengine-canada lacks a helper. - -The invariant pinned for helper variables: what a simulation calculates for -a later period does not depend on which earlier periods it calculated first. -Each such case compares against a fresh simulation that calculates only the -later period. The YEAR flow cases fail on 3.24.0 to 3.32.x; the integer, -boolean and STOCK year cases and the MONTH cases are controls that pass there -too. +A variable with a ``set_input`` helper now prefers periods at its definition +unit. The helper stores inputs at the definition period, so a value at +another unit beside them is ordinarily a cache derived from them. A value at +another unit can still be a genuine input: one stored before a reform changed +the variable's definition period or helper, one restored from a simulation +with other definitions, or one written by a custom helper. So other units +are used when the variable has nothing at its definition unit. Variables +without a helper (falsy ``set_input``: DAY variables, ``set_input = None``) +keep the rule of 3.24.0 to 3.32.x, the latest period of any unit. + +Known limits, pinned below: a helper variable that holds both a value at its +definition unit and a later genuine input at another unit carries the former; +a variable without a helper still carries a cached twelfth, as before. + +The order invariant pinned for YEAR inputs and MONTH flow inputs: what a +simulation calculates for a later period does not depend on which earlier +periods it calculated first. Each such case compares against a fresh +simulation that calculates only the later period. The YEAR flow cases fail on +3.24.0 to 3.32.x; the integer, boolean and STOCK year cases and the MONTH +cases are controls that pass there too. (A MONTH stock input asked for a +later year and then an earlier month of that year still gets the default for +the month; that predates this change.) """ from __future__ import annotations @@ -40,6 +48,8 @@ entities, situation_examples, ) +from policyengine_core.holders import set_input_dispatch_by_period +from policyengine_core.reforms import Reform from policyengine_core.simulations import SimulationBuilder from policyengine_core.variables import QuantityType, Variable @@ -86,6 +96,30 @@ class carried_monthly_flow(Variable): label = "Monthly flow input with no formula and no uprating" +class carried_monthly_stock(Variable): + value_type = float + entity = entities.Person + definition_period = periods.MONTH + quantity_type = QuantityType.STOCK + label = "Monthly stock input with no formula and no uprating" + + +class yearly_flow_with_dispatch_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + set_input = set_input_dispatch_by_period + label = "Yearly flow input whose helper copies rather than divides" + + +class yearly_flow_with_false_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + set_input = False + label = "Yearly flow input with a falsy set_input, treated as no helper" + + # Variables without a helper. @@ -132,26 +166,34 @@ class year_flow_input_without_helper(Variable): FLOW = "carried_flow" FLOW_INPUT = {FLOW: {BASE_YEAR: [120.0, 12.0]}} NO_HELPER_FLOW = "year_flow_input_without_helper" +ALL_VARIABLES = ( + carried_flow, + carried_stock, + carried_count, + carried_flag, + carried_monthly_flow, + carried_monthly_stock, + yearly_flow_with_dispatch_helper, + yearly_flow_with_false_helper, + day_stock_input, + month_stock_input_without_helper, + year_stock_input_without_helper, + year_flow_input_without_helper, +) -@pytest.fixture(scope="module") -def system(): +def _new_system(): system = CountryTaxBenefitSystem() system.auto_carry_over_input_variables = True - system.add_variables( - carried_flow, - carried_stock, - carried_count, - carried_flag, - carried_monthly_flow, - day_stock_input, - month_stock_input_without_helper, - year_stock_input_without_helper, - year_flow_input_without_helper, - ) + system.add_variables(*ALL_VARIABLES) return system +@pytest.fixture(scope="module") +def system(): + return _new_system() + + def _simulation(system, inputs): simulation = SimulationBuilder().build_from_entities( system, situation_examples.couple @@ -285,13 +327,37 @@ def test_no_carry_over_backwards_from_a_later_input(system): np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [0.0, 0.0]) -def test_variable_with_helper_never_carries_another_unit(system): - """Even when the value at another unit is all it holds.""" +def test_variable_with_helper_uses_another_unit_when_it_has_nothing_else(system): + """As before: core cannot tell this value from a genuine input.""" simulation = _simulation(system, {}) simulation.get_holder(FLOW).put_in_cache( np.array([10.0, 1.0]), periods.period(MONTHS[-1]) ) - np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [0.0, 0.0]) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [10.0, 1.0]) + + +def test_stock_month_cache_from_an_explicit_divide_does_not_carry(system): + simulation = _simulation(system, {"carried_stock": {BASE_YEAR: [40.0, 6.0]}}) + simulation.calculate_divide("carried_stock", MONTHS[-1]) + np.testing.assert_array_equal( + simulation.calculate("carried_stock", 2025), [40.0, 6.0] + ) + + +def test_flow_with_a_dispatch_helper_does_not_carry_a_twelfth(system): + variable = "yearly_flow_with_dispatch_helper" + simulation = _simulation(system, {variable: {BASE_YEAR: [120.0, 12.0]}}) + simulation.calculate(variable, MONTHS[-1]) + np.testing.assert_array_equal(simulation.calculate(variable, 2025), [120.0, 12.0]) + + +def test_month_cache_does_not_block_an_anchored_year_request(system): + """The early default return also looks only at the preferred periods.""" + simulation = _simulation(system, FLOW_INPUT) + simulation.calculate(FLOW, MONTHS[-1]) + np.testing.assert_array_equal( + simulation.calculate(FLOW, "year:2024-06"), [120.0, 12.0] + ) def test_branch_never_carries_an_inherited_twelfth(system): @@ -449,3 +515,112 @@ def test_variable_without_helper_still_carries_a_derived_twelfth(system): np.testing.assert_array_equal( simulation.calculate(NO_HELPER_FLOW, 2025), [10.0, 1.0] ) + + +def test_falsy_set_input_is_treated_as_no_helper(system): + variable = "yearly_flow_with_false_helper" + simulation = _simulation( + system, + {variable: {BASE_YEAR: [120.0, 12.0], MONTHS[-1]: [999.0, 999.0]}}, + ) + np.testing.assert_array_equal(simulation.calculate(variable, 2025), [999.0, 999.0]) + + +def test_helper_variable_with_both_units_carries_its_own_unit(system): + """Known limit: a genuine later input at another unit loses to a value at + the definition unit. Reached only when a value at another unit is a real + input, which the built-in helpers never store.""" + simulation = _simulation(system, FLOW_INPUT) + simulation.get_holder(FLOW).put_in_cache( + np.array([999.0, 999.0]), periods.period(MONTHS[-1]) + ) + np.testing.assert_array_equal(simulation.calculate(FLOW, 2025), [120.0, 12.0]) + + +# Supported paths that leave a helper variable with a genuine input at another +# unit. Each builds its own system: a reform must not touch the shared one. + + +def _yearly_stock_as_monthly(): + class carried_stock(Variable): + value_type = float + entity = entities.Person + definition_period = periods.MONTH + quantity_type = QuantityType.STOCK + label = "carried_stock, redefined monthly" + + return carried_stock + + +def _monthly_stock_as_yearly(): + class carried_monthly_stock(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + quantity_type = QuantityType.STOCK + label = "carried_monthly_stock, redefined yearly" + + return carried_monthly_stock + + +class YearlyStockBecomesMonthly(Reform): + def apply(self): + self.update_variable(_yearly_stock_as_monthly()) + + +class MonthlyStockBecomesYearly(Reform): + def apply(self): + self.update_variable(_monthly_stock_as_yearly()) + + +class NoHelperStockGetsHelper(Reform): + def apply(self): + class year_stock_input_without_helper(Variable): + value_type = float + entity = entities.Person + definition_period = periods.YEAR + quantity_type = QuantityType.STOCK + label = "Yearly stock input, now with the default helper" + + self.update_variable(year_stock_input_without_helper) + + +def test_input_kept_when_a_reform_makes_a_yearly_variable_monthly(): + simulation = _simulation(_new_system(), {"carried_stock": {BASE_YEAR: [40.0, 6.0]}}) + simulation.apply_reform(YearlyStockBecomesMonthly) + np.testing.assert_array_equal( + simulation.calculate("carried_stock", "2025-03"), [40.0, 6.0] + ) + + +def test_input_kept_when_a_reform_makes_a_monthly_variable_yearly(): + simulation = _simulation( + _new_system(), {"carried_monthly_stock": {BASE_YEAR: [3.0, 4.0]}} + ) + simulation.apply_reform(MonthlyStockBecomesYearly) + np.testing.assert_array_equal( + simulation.calculate("carried_monthly_stock", 2025), [3.0, 4.0] + ) + + +def test_input_kept_when_a_reform_gives_a_variable_a_helper(): + variable = "year_stock_input_without_helper" + simulation = _simulation(_new_system(), {variable: {MONTHS[-1]: [40.0, 6.0]}}) + simulation.apply_reform(NoHelperStockGetsHelper) + np.testing.assert_array_equal(simulation.calculate(variable, 2025), [40.0, 6.0]) + + +def test_input_kept_when_restored_into_a_system_with_another_period(tmp_path): + from policyengine_core.tools.simulation_dumper import ( + dump_simulation, + restore_simulation, + ) + + simulation = _simulation(_new_system(), {"carried_stock": {BASE_YEAR: [40.0, 6.0]}}) + dump_simulation(simulation, str(tmp_path / "dump")) + monthly_system = _new_system() + monthly_system.update_variable(_yearly_stock_as_monthly()) + restored = restore_simulation(str(tmp_path / "dump"), monthly_system) + np.testing.assert_array_equal( + restored.calculate("carried_stock", "2025-03"), [40.0, 6.0] + )