From ed1570fb3ca3a3e7fd9a7f5bbe1d7fd7a452e9b6 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 26 Sep 2026 08:12:59 -0400 Subject: [PATCH 1/3] Hold uprating index flat where undefined; uprate from latest earlier period A variable with `uprating` and no value for the requested period is uprated from its latest known earlier period by the ratio of the uprating parameter at the two period starts. Two bugs lived in that step: - The parameter can have no value at the earlier instant (an input given for a year before the index starts). The ratio then divided by None and raised TypeError. The index is now held flat where it has no value: its first value before it starts, the last value before an explicit null after one. A value known for a period the index does not reach is carried over unchanged until the index starts, then uprated with it. - The earlier period was chosen by indexing the unfiltered known-periods list with a position in the filtered list, so a later period stored first was used instead of the latest earlier one. Co-Authored-By: Claude Opus 5.5 --- .../fix-uprating-undefined-index.fixed.md | 1 + policyengine_core/simulations/simulation.py | 57 ++++- .../core/variables/test_variable_uprating.py | 226 ++++++++++++++++++ 3 files changed, 277 insertions(+), 7 deletions(-) create mode 100644 changelog.d/fix-uprating-undefined-index.fixed.md create mode 100644 tests/core/variables/test_variable_uprating.py diff --git a/changelog.d/fix-uprating-undefined-index.fixed.md b/changelog.d/fix-uprating-undefined-index.fixed.md new file mode 100644 index 00000000..9f892727 --- /dev/null +++ b/changelog.d/fix-uprating-undefined-index.fixed.md @@ -0,0 +1 @@ +Uprating an input variable no longer raises `TypeError` when its uprating parameter has no value at the known period's start (the index is held flat where it has no value), and it now always uprates from the latest known earlier period rather than one picked by list position. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 0d4e001c..73efd382 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -44,6 +44,35 @@ def _stable_hash_to_seed(value: str) -> int: return int.from_bytes(digest[:4], "big") % 1000000 +def _uprating_index_value(parameter, instant) -> Optional[float]: + """Value of a variable's uprating index at ``instant``, held flat where + the index has no value. + + Before the index's first value this returns that first value, the same + convention as extending a parameter's earliest value backward. After an + explicit null it returns the last value before the null. A value known + for a period the index does not reach is therefore carried over + unchanged until the index starts, then uprated with it. Returns ``None`` + only if the index has no non-null value at all. + """ + value = parameter(instant) + if value is not None: + return value + defined = [ + value_at_instant + for value_at_instant in getattr(parameter, "values_list", []) + if value_at_instant.value is not None + ] + if not defined: + return None + # ``values_list`` runs from the latest instant to the earliest. + instant_str = str(instant) + for value_at_instant in defined: + if value_at_instant.instant_str <= instant_str: + return value_at_instant.value + return defined[-1].value + + if TYPE_CHECKING: from policyengine_core.taxbenefitsystems import TaxBenefitSystem @@ -836,14 +865,20 @@ 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() - start_instants = [ - str(known_period.start) + 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 variable.uprating is not None and len(start_instants) > 0: - latest_known_period = known_periods[np.argmax(start_instants)] + if variable.uprating is not None and len(earlier_known_periods) > 0: + # Take the latest period from the filtered list itself. + # Indexing ``known_periods`` with a position in the + # filtered list picked the wrong period whenever a later + # one was stored first. + latest_known_period = max( + earlier_known_periods, key=lambda p: p.start + ) try: uprating_parameter = get_parameter( self.tax_benefit_system.parameters, @@ -853,9 +888,17 @@ def _calculate(self, variable_name: str, period: Period = None) -> ArrayLike: raise ValueError( f"Could not find uprating parameter {variable.uprating} when trying to uprate {variable_name}." ) - value_in_last_period = uprating_parameter(latest_known_period.start) - value_in_this_period = uprating_parameter(period.start) - if value_in_last_period == 0: + value_in_last_period = _uprating_index_value( + uprating_parameter, latest_known_period.start + ) + value_in_this_period = _uprating_index_value( + uprating_parameter, period.start + ) + if ( + value_in_last_period is None + or value_in_this_period is None + or value_in_last_period == 0 + ): uprating_factor = 1 else: uprating_factor = value_in_this_period / value_in_last_period diff --git a/tests/core/variables/test_variable_uprating.py b/tests/core/variables/test_variable_uprating.py new file mode 100644 index 00000000..dd5bf813 --- /dev/null +++ b/tests/core/variables/test_variable_uprating.py @@ -0,0 +1,226 @@ +"""Uprating a variable from its latest known earlier period. + +A variable with ``uprating`` and no value for the requested period takes its +latest known earlier value and multiplies it by the ratio of the uprating +index at the two period starts. Two bugs lived in that step: + +* The index can have no value at the earlier instant (a value supplied for a + year before the index starts). The ratio then divided by ``None`` and + raised ``TypeError``. The index is now held flat where it has no value. +* The earlier period was chosen by indexing the unfiltered list of known + periods with a position in the filtered list, so a later period stored + first was used instead of the latest earlier one. +""" + +import itertools +import math + +import pytest + +from policyengine_core.country_template import CountryTaxBenefitSystem +from policyengine_core.country_template.entities import Person +from policyengine_core.model_api import MONTH, YEAR, Variable +from policyengine_core.parameters import ParameterNode +from policyengine_core.periods import Instant, Period +from policyengine_core.simulations import SimulationBuilder +from policyengine_core.simulations.simulation import _uprating_index_value + +INDEX_START = 2015 +# The index grows 10% a year from INDEX_START. Integer powers keep the +# expected ratios exact. +INDEX = { + f"{year}-01-01": 100 * 1.1 ** (year - INDEX_START) for year in range(2015, 2021) +} +VALUE = 5_000.0 + + +def build_system(index_values: dict) -> CountryTaxBenefitSystem: + system = CountryTaxBenefitSystem() + system.parameters.add_child( + "test_uprating", + ParameterNode("test_uprating", data={"index": {"values": index_values}}), + ) + + class uprated_income(Variable): + value_type = float + entity = Person + definition_period = YEAR + label = "Uprated yearly income" + uprating = "test_uprating.index" + + class uprated_monthly_income(Variable): + value_type = float + entity = Person + definition_period = MONTH + label = "Uprated monthly income" + uprating = "test_uprating.index" + + system.add_variable(uprated_income) + system.add_variable(uprated_monthly_income) + return system + + +@pytest.fixture(scope="module") +def system(): + return build_system(INDEX) + + +@pytest.fixture(scope="module") +def backdated_system(): + """The same index with its first value explicitly extended back to 2000, + the way a country package backdates parameters.""" + system = build_system(INDEX) + index = system.parameters.test_uprating.index + first = f"{INDEX_START}-01-01" + days = (Instant((INDEX_START, 1, 1)).date - Instant((2000, 1, 1)).date).days + index.update( + period=Period(("day", Instant((2000, 1, 1)), days)), + value=index(first), + ) + assert index("2000-01-01") == index(first) + return system + + +def simulate(system, inputs: dict, variable: str = "uprated_income"): + return SimulationBuilder().build_from_entities( + system, {"persons": {"p": {variable: inputs}}} + ) + + +def calculate(system, inputs: dict, period, variable: str = "uprated_income"): + return float(simulate(system, inputs, variable).calculate(variable, period)[0]) + + +def index_at(year: int) -> float: + """The index held flat before INDEX_START, as the fix reads it.""" + return INDEX[f"{min(max(year, INDEX_START), 2020)}-01-01"] + + +# Regression tests. + + +def test_value_known_before_index_starts_is_uprated_from_index_start(system): + # Before the fix: TypeError: unsupported operand type(s) for /: 'float' + # and 'NoneType'. + assert calculate(system, {2013: VALUE}, 2018) == pytest.approx( + VALUE * INDEX["2018-01-01"] / INDEX["2015-01-01"] + ) + + +def test_value_carried_over_unchanged_while_index_undefined(system): + # Before the fix: TypeError dividing None by None. + assert calculate(system, {2012: VALUE}, 2014) == pytest.approx(VALUE) + assert calculate(system, {2012: VALUE}, 2015) == pytest.approx(VALUE) + + +def test_monthly_value_known_before_index_starts(system): + assert calculate( + system, {"2014-12": VALUE}, "2016-01", "uprated_monthly_income" + ) == pytest.approx(VALUE * INDEX["2016-01-01"] / INDEX["2015-01-01"]) + + +def test_uprates_from_latest_earlier_period_when_later_period_stored_first( + system, +): + # Before the fix the 2020 value (stored first) was used: 999. + inputs = {2020: 999.0, 2015: VALUE} + assert calculate(system, inputs, 2018) == pytest.approx( + VALUE * INDEX["2018-01-01"] / INDEX["2015-01-01"] + ) + + +def test_index_gap_from_explicit_null_is_held_flat(): + # ``update`` on a period before an index starts leaves an explicit null + # between the new value and the old first value. + system = build_system(INDEX) + index = system.parameters.test_uprating.index + index.update(period="2010", value=50) + assert index("2012-01-01") is None + # 2012 falls in the gap: the index is held at its 2010 value there. + assert calculate(system, {2012: VALUE}, 2016) == pytest.approx( + VALUE * INDEX["2016-01-01"] / 50 + ) + assert calculate(system, {2011: VALUE}, 2013) == pytest.approx(VALUE) + + +def test_index_with_no_values_carries_value_over(): + system = build_system({"2015-01-01": None}) + assert calculate(system, {2013: VALUE}, 2018) == pytest.approx(VALUE) + + +def test_index_value_helper(): + system = build_system(INDEX) + index = system.parameters.test_uprating.index + index.update(period="2030", value=None) + assert _uprating_index_value(index, Instant((2010, 6, 1))) == INDEX["2015-01-01"] + assert _uprating_index_value(index, Instant((2017, 1, 1))) == INDEX["2017-01-01"] + # After an explicit null the last value before it holds. + assert index("2030-06-01") is None + assert _uprating_index_value(index, Instant((2030, 6, 1))) == INDEX["2020-01-01"] + assert _uprating_index_value(index, Instant((2040, 1, 1))) == INDEX["2020-01-01"] + empty = build_system({"2015-01-01": None}).parameters.test_uprating.index + assert _uprating_index_value(empty, Instant((2016, 1, 1))) is None + + +# Invariants, checked exhaustively over a grid of known and requested years +# (2008-2022), which spans the index start and its last value. + +YEARS = range(2008, 2023) +PAIRS = [(known, requested) for known, requested in itertools.combinations(YEARS, 2)] + + +@pytest.mark.parametrize("known,requested", PAIRS) +def test_uprated_value_equals_value_times_flat_index_ratio(system, known, requested): + """For every known year before every requested year, the result is the + known value times the ratio of the index held flat where undefined. Where + the index is defined at both years this is the ratio uprating always + used; where it is undefined at both the value carries over unchanged.""" + result = calculate(system, {known: VALUE}, requested) + expected = VALUE * index_at(requested) / index_at(known) + assert math.isfinite(result) + assert result == pytest.approx(expected, rel=1e-6) + if requested <= INDEX_START: + assert result == pytest.approx(VALUE, rel=1e-6) + + +@pytest.mark.parametrize("known,requested", PAIRS) +def test_matches_explicitly_backdated_index(system, backdated_system, known, requested): + """Differential: holding the index flat before it starts gives the same + result as a country package explicitly extending the index's first value + backward and uprating through the ordinary defined-index path.""" + assert calculate(system, {known: VALUE}, requested) == pytest.approx( + calculate(backdated_system, {known: VALUE}, requested), rel=1e-6 + ) + + +@pytest.mark.parametrize("known", range(2008, 2016)) +def test_result_does_not_depend_on_intermediate_years_computed(system, known): + """Path independence: computing every year in turn (each uprating from the + one before, which is then known) gives the same final value as uprating + straight from the known year.""" + stepwise = simulate(system, {known: VALUE}) + for year in range(known + 1, 2022): + stepwise.calculate("uprated_income", year) + stepwise_result = float(stepwise.calculate("uprated_income", 2022)[0]) + assert stepwise_result == pytest.approx( + calculate(system, {known: VALUE}, 2022), rel=1e-5 + ) + + +@pytest.mark.parametrize( + "stored_years", list(itertools.permutations([2012, 2016, 2019])) +) +def test_latest_earlier_period_wins_regardless_of_storage_order(system, stored_years): + """Order independence: for any insertion order of the known periods, each + requested year uprates from the latest known year before it.""" + inputs = {year: 1_000.0 * (i + 1) for i, year in enumerate(stored_years)} + for requested in (2014, 2017, 2021): + latest = max(year for year in inputs if year < requested) + assert calculate(system, inputs, requested) == pytest.approx( + inputs[latest] * index_at(requested) / index_at(latest), rel=1e-6 + ) + + +def test_value_known_after_requested_period_is_not_used(system): + # Uprating only runs forward; a later value alone gives the default. + assert calculate(system, {2018: VALUE}, 2016) == 0 From 753cc4eff1c0d41f9bf2fcf9558b1daf385428a3 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 26 Sep 2026 08:31:13 -0400 Subject: [PATCH 2/3] Test evaluation-order independence; document variable uprating rule A lagged-year lookup caches the default at an earlier year; a later request between that year and the input year then deflated the later input on master. Add that regression case and describe the uprating rule, including the flat index where the parameter has no value, on Variable.uprating. Co-Authored-By: Claude Opus 5.5 --- policyengine_core/variables/variable.py | 2 +- tests/core/variables/test_variable_uprating.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/policyengine_core/variables/variable.py b/policyengine_core/variables/variable.py index 0c2e2701..00972cd5 100644 --- a/policyengine_core/variables/variable.py +++ b/policyengine_core/variables/variable.py @@ -123,7 +123,7 @@ class Variable: """List of variables that are subtracted from the variable. Alternatively, can be a parameter name.""" uprating: str = None - """Name of a parameter used to uprate the variable.""" + """Name of a parameter used to uprate the variable. When the variable has no value for a requested period, its value in the latest known earlier period is multiplied by the ratio of this parameter at the two period starts. Where the parameter has no value it is held flat: before its first value it takes that first value, and after an explicit null it keeps the last value before the null. The variable therefore carries over unchanged across any span the parameter does not cover.""" hidden_input: bool = False """Whether the variable is hidden from the input screen entirely on PolicyEngine.""" diff --git a/tests/core/variables/test_variable_uprating.py b/tests/core/variables/test_variable_uprating.py index dd5bf813..610c0ea2 100644 --- a/tests/core/variables/test_variable_uprating.py +++ b/tests/core/variables/test_variable_uprating.py @@ -129,6 +129,18 @@ def test_uprates_from_latest_earlier_period_when_later_period_stored_first( ) +def test_cached_default_before_input_does_not_pull_later_input_back(system): + # A formula reading an earlier year (a lagged-income lookup, say) caches + # the default there. Before the fix a later request between that year + # and the input year then deflated the later input instead of using the + # latest earlier period, so the result depended on evaluation order. + direct = calculate(system, {2018: VALUE}, 2017) + lagged_first = simulate(system, {2018: VALUE}) + lagged_first.calculate("uprated_income", 2016) + assert direct == 0 + assert float(lagged_first.calculate("uprated_income", 2017)[0]) == direct + + def test_index_gap_from_explicit_null_is_held_flat(): # ``update`` on a period before an index starts leaves an explicit null # between the new value and the old first value. From b95c86bde75eedcb0fb431f27289440a021d90c4 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 27 Sep 2026 05:42:26 -0400 Subject: [PATCH 3/3] Address review: isolate base-period test, open-ended null, docs - Storage-order test uses years inside the index range so it tests the base-period choice alone (5 of 6 orders fail on master). - The helper test sets an open-ended null, so the hold-after-null path is exercised at 2040, and imports the helper locally so the module runs against code without it. - Docstrings: the backward hold mirrors country-package backdating (a parameter itself returns None there); cached values count as known and a zero base carries over. Changelog covers a missing value at either period start. Co-Authored-By: Claude Opus 5.5 --- .../fix-uprating-undefined-index.fixed.md | 2 +- policyengine_core/simulations/simulation.py | 5 +++-- policyengine_core/variables/variable.py | 2 +- tests/core/variables/test_variable_uprating.py | 16 +++++++++++----- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/changelog.d/fix-uprating-undefined-index.fixed.md b/changelog.d/fix-uprating-undefined-index.fixed.md index 9f892727..e715fca8 100644 --- a/changelog.d/fix-uprating-undefined-index.fixed.md +++ b/changelog.d/fix-uprating-undefined-index.fixed.md @@ -1 +1 @@ -Uprating an input variable no longer raises `TypeError` when its uprating parameter has no value at the known period's start (the index is held flat where it has no value), and it now always uprates from the latest known earlier period rather than one picked by list position. +Uprating an input variable no longer raises `TypeError` when its uprating parameter has no value at the start of the known or the requested period (the parameter is held flat where it has no value), and it now always uprates from the latest known earlier period rather than one picked by list position. diff --git a/policyengine_core/simulations/simulation.py b/policyengine_core/simulations/simulation.py index 73efd382..566407a5 100644 --- a/policyengine_core/simulations/simulation.py +++ b/policyengine_core/simulations/simulation.py @@ -48,8 +48,9 @@ def _uprating_index_value(parameter, instant) -> Optional[float]: """Value of a variable's uprating index at ``instant``, held flat where the index has no value. - Before the index's first value this returns that first value, the same - convention as extending a parameter's earliest value backward. After an + Before the index's first value this returns that first value, as if the + earliest value had been extended backward (country packages backdate + parameters this way; a parameter itself returns ``None`` there). After an explicit null it returns the last value before the null. A value known for a period the index does not reach is therefore carried over unchanged until the index starts, then uprated with it. Returns ``None`` diff --git a/policyengine_core/variables/variable.py b/policyengine_core/variables/variable.py index 00972cd5..629a1f4f 100644 --- a/policyengine_core/variables/variable.py +++ b/policyengine_core/variables/variable.py @@ -123,7 +123,7 @@ class Variable: """List of variables that are subtracted from the variable. Alternatively, can be a parameter name.""" uprating: str = None - """Name of a parameter used to uprate the variable. When the variable has no value for a requested period, its value in the latest known earlier period is multiplied by the ratio of this parameter at the two period starts. Where the parameter has no value it is held flat: before its first value it takes that first value, and after an explicit null it keeps the last value before the null. The variable therefore carries over unchanged across any span the parameter does not cover.""" + """Name of a parameter used to uprate the variable. When the variable has no value for a requested period, its value in the latest known earlier period (an input, or a value already calculated or defaulted and cached) is multiplied by the ratio of this parameter at the two period starts, or carried over unchanged if the parameter is zero at the earlier start. Where the parameter has no value it is held flat: before its first value it takes that first value, and after an explicit null it keeps the last value before the null. The variable therefore carries over unchanged across any span the parameter does not cover.""" hidden_input: bool = False """Whether the variable is hidden from the input screen entirely on PolicyEngine.""" diff --git a/tests/core/variables/test_variable_uprating.py b/tests/core/variables/test_variable_uprating.py index 610c0ea2..eb634a92 100644 --- a/tests/core/variables/test_variable_uprating.py +++ b/tests/core/variables/test_variable_uprating.py @@ -23,7 +23,6 @@ from policyengine_core.parameters import ParameterNode from policyengine_core.periods import Instant, Period from policyengine_core.simulations import SimulationBuilder -from policyengine_core.simulations.simulation import _uprating_index_value INDEX_START = 2015 # The index grows 10% a year from INDEX_START. Integer powers keep the @@ -161,9 +160,14 @@ def test_index_with_no_values_carries_value_over(): def test_index_value_helper(): + # Imported here so the rest of the module also runs against code without + # the helper. + from policyengine_core.simulations.simulation import _uprating_index_value + system = build_system(INDEX) index = system.parameters.test_uprating.index - index.update(period="2030", value=None) + # A null from 2030 with no end date: the index is undefined from then on. + index.update(start="2030-01-01", value=None) assert _uprating_index_value(index, Instant((2010, 6, 1))) == INDEX["2015-01-01"] assert _uprating_index_value(index, Instant((2017, 1, 1))) == INDEX["2017-01-01"] # After an explicit null the last value before it holds. @@ -220,13 +224,15 @@ def test_result_does_not_depend_on_intermediate_years_computed(system, known): @pytest.mark.parametrize( - "stored_years", list(itertools.permutations([2012, 2016, 2019])) + "stored_years", list(itertools.permutations([2015, 2017, 2019])) ) def test_latest_earlier_period_wins_regardless_of_storage_order(system, stored_years): """Order independence: for any insertion order of the known periods, each - requested year uprates from the latest known year before it.""" + requested year uprates from the latest known year before it. All years + are inside the index's range, so only the choice of base period is + tested here.""" inputs = {year: 1_000.0 * (i + 1) for i, year in enumerate(stored_years)} - for requested in (2014, 2017, 2021): + for requested in (2016, 2018, 2020): latest = max(year for year in inputs if year < requested) assert calculate(system, inputs, requested) == pytest.approx( inputs[latest] * index_at(requested) / index_at(latest), rel=1e-6