diff --git a/changelog.d/499.fixed.md b/changelog.d/499.fixed.md new file mode 100644 index 00000000..4e34fcea --- /dev/null +++ b/changelog.d/499.fixed.md @@ -0,0 +1,3 @@ +Fixed `Parameter.parameter_values` to return oldest-first, non-overlapping +histories with inclusive end dates instead of reversed intervals derived from +PolicyEngine Core's newest-first storage. diff --git a/docs/reference/index.md b/docs/reference/index.md index 7699176d..cf7c36fa 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -4,6 +4,14 @@ title: "Reference" Reference pages are generated from the installed country-model packages. Authored methodology pages explain why the model is structured the way it is; generated reference pages expose the exact release contents. +## Parameter history contract + +`model.get_parameter(name).parameter_values` exposes effective values from +oldest to newest. Each bounded value has an inclusive `end_date` equal to the +day before the next value starts, while the newest value has `end_date=None`. +If the underlying Core history contains duplicate effective starts, the public +history contains the first value Core would select for that date. + ## What generated reference should include The variable reference generator already reads the installed country model and can emit: diff --git a/src/policyengine/core/parameter.py b/src/policyengine/core/parameter.py index 49f2b282..680120cf 100644 --- a/src/policyengine/core/parameter.py +++ b/src/policyengine/core/parameter.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta from typing import TYPE_CHECKING, Any, Optional from uuid import uuid4 @@ -32,25 +33,39 @@ def __init__(self, _core_param: Any = None, **data): @property def parameter_values(self) -> list["ParameterValue"]: - """Lazily build parameter values on first access.""" + """Lazily build the effective parameter history on first access. + + Values are ordered from oldest to newest. ``end_date`` is inclusive, + so each bounded value ends one day before the next value starts and the + newest value remains open-ended. When Core exposes duplicate effective + starts, retain the first entry because Core's lookup uses the first + matching entry as the effective value. + """ if self._parameter_values is None: self._parameter_values = [] if self._core_param is not None: from policyengine.utils import parse_safe_date - for i in range(len(self._core_param.values_list)): - param_at_instant = self._core_param.values_list[i] - if i + 1 < len(self._core_param.values_list): - next_instant = self._core_param.values_list[i + 1] - else: - next_instant = None + effective_values: dict[datetime, Any] = {} + for value_at_instant in self._core_param.values_list: + start_date = parse_safe_date(value_at_instant.instant_str) + effective_values.setdefault(start_date, value_at_instant) + + chronological_values = sorted(effective_values.items()) + for index, (start_date, value_at_instant) in enumerate( + chronological_values + ): + next_index = index + 1 + end_date = ( + chronological_values[next_index][0] - timedelta(days=1) + if next_index < len(chronological_values) + else None + ) pv = ParameterValue( parameter=self, - start_date=parse_safe_date(param_at_instant.instant_str), - end_date=parse_safe_date(next_instant.instant_str) - if next_instant - else None, - value=param_at_instant.value, + start_date=start_date, + end_date=end_date, + value=value_at_instant.value, ) self._parameter_values.append(pv) return self._parameter_values diff --git a/tests/test_models.py b/tests/test_models.py index 7e05a69a..1b1066e8 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,11 +1,25 @@ """Tests for UK and US tax-benefit model versions and core models.""" import re +from datetime import timedelta from policyengine.tax_benefit_models.uk import uk_latest from policyengine.tax_benefit_models.us import us_latest +def _assert_parameter_value_interval_contract(model, parameter_name): + values = model.get_parameter(parameter_name).parameter_values + + assert len(values) > 1 + assert [value.start_date for value in values] == sorted( + value.start_date for value in values + ) + for current, following in zip(values, values[1:]): + assert current.end_date == following.start_date - timedelta(days=1) + assert current.start_date <= current.end_date + assert values[-1].end_date is None + + class TestUKModel: """Tests for PolicyEngine UK model.""" @@ -70,6 +84,13 @@ def test_model_version_parameter_values_aggregates_all(self): all_values = uk_latest.parameter_values assert len(all_values) >= 100 + def test_parameter_value_intervals_are_chronological_and_inclusive(self): + """UK catalog histories should expose valid inclusive intervals.""" + _assert_parameter_value_interval_contract( + uk_latest, + "gov.hmrc.income_tax.rates.uk[0].rate", + ) + def test__given_bracket_parameter__then_has_generated_label(self): """Bracket parameters should have auto-generated labels.""" bracket_params_with_labels = [ @@ -155,6 +176,13 @@ def test_model_version_parameter_values_aggregates_all(self): all_values = us_latest.parameter_values assert len(all_values) >= 100 + def test_parameter_value_intervals_are_chronological_and_inclusive(self): + """US catalog histories should expose valid inclusive intervals.""" + _assert_parameter_value_interval_contract( + us_latest, + "gov.irs.credits.ctc.amount.base[0].amount", + ) + def test__given_breakdown_parameter__then_has_generated_label(self): """Breakdown parameters (e.g., filing status) should have auto-generated labels.""" breakdown_params_with_labels = [ diff --git a/tests/test_parameter_values.py b/tests/test_parameter_values.py new file mode 100644 index 00000000..4d6753da --- /dev/null +++ b/tests/test_parameter_values.py @@ -0,0 +1,138 @@ +"""Tests for parameter history conversion from PolicyEngine Core.""" + +from datetime import datetime +from types import SimpleNamespace + +from policyengine.core.parameter import Parameter +from policyengine.core.parameter_value import ParameterValue + + +def _parameter_with_core_values(*values: tuple[str, object]) -> Parameter: + parameter = Parameter.model_construct( + name="gov.test.value", + tax_benefit_model_version=None, + ) + parameter._core_param = SimpleNamespace( + values_list=[ + SimpleNamespace(instant_str=start_date, value=value) + for start_date, value in values + ] + ) + parameter._parameter_values = None + return parameter + + +def test__newest_first_core_values__then_history_is_chronological_and_inclusive(): + parameter = _parameter_with_core_values( + ("2024-07-15", 30), + ("2022-03-10", 20), + ("2020-01-01", 10), + ) + + values = parameter.parameter_values + + assert [value.start_date for value in values] == [ + datetime(2020, 1, 1), + datetime(2022, 3, 10), + datetime(2024, 7, 15), + ] + assert [value.end_date for value in values] == [ + datetime(2022, 3, 9), + datetime(2024, 7, 14), + None, + ] + assert [value.value for value in values] == [10, 20, 30] + assert all(value.parameter is parameter for value in values) + + +def test__single_core_value__then_history_is_open_ended(): + parameter = _parameter_with_core_values(("2024-01-01", 10)) + + (value,) = parameter.parameter_values + + assert value.start_date == datetime(2024, 1, 1) + assert value.end_date is None + + +def test__consecutive_day_updates__then_earlier_value_covers_one_day(): + parameter = _parameter_with_core_values( + ("2024-01-02", 20), + ("2024-01-01", 10), + ) + + values = parameter.parameter_values + + assert values[0].start_date == datetime(2024, 1, 1) + assert values[0].end_date == datetime(2024, 1, 1) + assert values[0].value == 10 + assert values[1].start_date == datetime(2024, 1, 2) + assert values[1].end_date is None + assert values[1].value == 20 + + +def test__duplicate_effective_starts__then_first_core_value_wins(): + parameter = _parameter_with_core_values( + ("2024-01-01", "effective"), + ("2024-01-01", "shadowed"), + ("2020-01-01", "older"), + ) + + values = parameter.parameter_values + + assert len(values) == 2 + assert values[0].value == "older" + assert values[0].end_date == datetime(2023, 12, 31) + assert values[1].value == "effective" + assert values[1].end_date is None + + +def test__year_zero_core_value__then_existing_safe_date_normalization_is_preserved(): + parameter = _parameter_with_core_values( + ("2020-01-01", 20), + ("0000-01-01", 10), + ) + + values = parameter.parameter_values + + assert values[0].start_date == datetime(1, 1, 1) + assert values[0].end_date == datetime(2019, 12, 31) + assert values[1].start_date == datetime(2020, 1, 1) + assert values[1].end_date is None + + +def test__repeated_access__then_parameter_history_is_cached(): + parameter = _parameter_with_core_values(("2024-01-01", 10)) + + first = parameter.parameter_values + + assert parameter.parameter_values is first + + +def test__explicit_parameter_values__then_setter_override_is_preserved(): + parameter = _parameter_with_core_values(("2024-01-01", 10)) + override = [ + ParameterValue( + parameter=parameter, + value=99, + start_date=datetime(2030, 1, 1), + end_date=None, + ) + ] + + parameter.parameter_values = override + + assert parameter.parameter_values is override + + +def test__parameter_without_core_reference__then_history_is_empty(): + parameter = Parameter.model_construct( + name="gov.test.value", + tax_benefit_model_version=None, + ) + parameter._core_param = None + parameter._parameter_values = None + + values = parameter.parameter_values + + assert values == [] + assert parameter.parameter_values is values diff --git a/tests/test_parametric_reforms.py b/tests/test_parametric_reforms.py index 3c752226..f0062804 100644 --- a/tests/test_parametric_reforms.py +++ b/tests/test_parametric_reforms.py @@ -1,7 +1,9 @@ """Tests for parametric reforms utility functions.""" from datetime import date +from types import SimpleNamespace +from policyengine.core.parameter import Parameter from policyengine.utils.parametric_reforms import ( reform_dict_from_parameter_values, simulation_modifier_from_parameter_values, @@ -226,6 +228,29 @@ def test__given_open_ended_and_explicit_end_values__then_open_ended_clips_at_nex "2026-01-01.2026-12-31": 200, } + def test__given_catalog_history__then_emits_valid_inclusive_ranges(self): + """Catalog intervals should preserve inclusive reform boundaries.""" + parameter = Parameter.model_construct( + name="gov.test.catalog_value", + tax_benefit_model_version=None, + ) + parameter._core_param = SimpleNamespace( + values_list=[ + SimpleNamespace(instant_str="2022-07-01", value=20), + SimpleNamespace(instant_str="2020-01-01", value=10), + ] + ) + parameter._parameter_values = None + + result = reform_dict_from_parameter_values(parameter.parameter_values) + + assert result == { + "gov.test.catalog_value": { + "2020-01-01.2022-06-30": 10, + "2022-07-01.2100-12-31": 20, + } + } + class TestSimulationModifierFromParameterValues: """Tests for the simulation_modifier_from_parameter_values function."""