diff --git a/changelog.d/fix-own-period-input-stale-sums.fixed.md b/changelog.d/fix-own-period-input-stale-sums.fixed.md new file mode 100644 index 000000000..97ee376eb --- /dev/null +++ b/changelog.d/fix-own-period-input-stale-sums.fixed.md @@ -0,0 +1 @@ +`set_input` for one of a variable's own periods (one month, or one year) now drops the sums and twelfths the simulation calculated for that variable over overlapping periods, so a later `calculate` adds up or divides the new input instead of returning the old result (#579). diff --git a/docs/usage/branches.md b/docs/usage/branches.md index 1465c01e7..8a103f6fc 100644 --- a/docs/usage/branches.md +++ b/docs/usage/branches.md @@ -149,18 +149,19 @@ therefore settle without exhausting the retry budget. An input set again with the value the branch already reads for that very period, as an input (not through a `set_input` helper that spreads it over -other periods, and not `-0.0` for `0.0`), changes nothing, so it drops nothing -and is no input change. An input stored for the very period being calculated -after the calculation began (by its own formula, say; under the branch's name -or any it reads, such as `default` through `Holder.set_input`) is the result, +other periods, and not `-0.0` for `0.0`), does not trigger branch dependency +invalidation or count as an input change. An input stored for the very period +being calculated after the calculation began (by its own formula, say; under the +branch's name or any it reads, such as `default` through `Holder.set_input`) is the result, as it would be had it been set first. Input helpers that divide or dispatch a larger-period input replace calculated sub-periods while retaining supplied inputs in their storage tiers. Their shared -overlap cleanup also removes calculated sums or twelfths for the variable, -including inherited values in the branch's storage, without changing the parent. +overlap cleanup also runs for an input at a non-eternal variable's own period and +removes calculated sums or twelfths for the variable, including inherited values +in the branch's storage, without changing the parent. Branch dependency invalidation additionally drops later calculated values that -may depend on the replaced input. On a simulation without a parent, helper +may depend on the replaced input. On a simulation without a parent, overlap cleanup still runs while branch dependency invalidation does not. A custom `set_input` handler that calculates values between its own stores @@ -169,9 +170,9 @@ anything (through `calculate`, `calculate_add` or `_calculate`), the branch drops again, by the same rule, once the handler returns or raises (the inputs it stored before raising stay). -If there is no such record, the branch drops nothing. That is the case when a -formula creates the branch while it is still calculating the variable the -branch overrides, unless another branch the formula created for the same +If there is no such record, branch dependency invalidation drops nothing. That is +the case when a formula creates the branch while it is still calculating the +variable the branch overrides, unless another branch the formula created for the same comparison already handed back a value calculated from that variable: then the branch drops what came back and what was calculated after it. @@ -200,8 +201,8 @@ or carried-over value also counts. A result whose calculation was running when an input changed is not kept unless it was calculated again from the new inputs. An input set again with the value the branch already reads for that period, as an input, changes no value read anywhere, so -dropping nothing then is sound; one set where the value was calculated still -drops, since it changes which periods hold inputs. +skipping dependency invalidation then is sound; one set where the value was +calculated still drops, since it changes which periods hold inputs. The rule can drop more than it needs to (a value stored later that does not depend on the input is calculated again) but not less, within these limits: diff --git a/policyengine_core/holders/helpers.py b/policyengine_core/holders/helpers.py index b6e4b9828..1eba51fff 100644 --- a/policyengine_core/holders/helpers.py +++ b/policyengine_core/holders/helpers.py @@ -167,6 +167,56 @@ def _drop_calculated_overlapping( _evict_fast_cache(holder, dropped_periods, branch_name) +def _drop_calculated_over_own_period( + holder: Holder, period: Period, branch_name: str +) -> None: + """Drop what the simulation calculated for the variable over periods + that overlap ``period``, once an input is set for ``period``, one of the + variable's own periods (policyengine-core#579). + + ``calculate`` adds up a monthly flow over a year, or divides a yearly + flow into a month, and caches the result at that period. The value the + input replaced went into it, so it is out of date, and ``calculate`` + would return it instead of adding up or dividing the input. The rule is + the one the ``set_input`` helpers follow (``_drop_calculated_overlapping``), + applied to an input that needs no helper. Inputs are kept. + + ``calculate``'s fast cache also drops what it holds for the variable at + periods that overlap ``period``, ``period`` included, so an input set + through the holder rather than ``Simulation.set_input`` is read back too. + """ + _drop_calculated_overlapping(holder, period, branch_name) + simulation = getattr(holder, "simulation", None) + fast_cache = getattr(simulation, "_fast_cache", None) + if not fast_cache: + return + name = holder.variable.name + _evict_fast_cache( + holder, + [ + cached_period + for cached_name, cached_period in list(fast_cache) + if cached_name == name + and isinstance(cached_period, Period) + and cached_period.unit != periods.ETERNITY + and _periods_overlap(cached_period, period) + ], + branch_name, + ) + + +def _periods_overlap(first: Period, second: Period) -> bool: + """Compare extents even when a period ends beyond datetime's last date.""" + try: + return not (first.start > second.stop or first.stop < second.start) + except OverflowError: + from policyengine_core.simulations.simulation import _end_order + + return first.start.date.toordinal() < _end_order( + second + ) and second.start.date.toordinal() < _end_order(first) + + def _is_calculated_overlapping( holder: Holder, stored_branch_name: str, @@ -184,7 +234,7 @@ def _is_calculated_overlapping( return False if stored_period.unit == periods.ETERNITY: return False - if stored_period.start > period.stop or stored_period.stop < period.start: + if not _periods_overlap(stored_period, period): return False return not _is_input(holder, stored_period, stored_branch_name, storage_name) diff --git a/policyengine_core/holders/holder.py b/policyengine_core/holders/holder.py index 91c52b8a8..4b3c117fb 100644 --- a/policyengine_core/holders/holder.py +++ b/policyengine_core/holders/holder.py @@ -652,7 +652,14 @@ def set_input( simulation._drop_values_that_may_depend_on( self.variable.name, period ) - return self._set(period, array, branch_name, validate_nan=True) + self._set(period, array, branch_name, validate_nan=True) + if self.variable.definition_period != periods.ETERNITY: + # Imported here because the helpers module imports this one. + from policyengine_core.holders.helpers import ( + _drop_calculated_over_own_period, + ) + + _drop_calculated_over_own_period(self, period, branch_name) finally: if simulation is not None: simulation._user_input_contexts.pop() diff --git a/tests/core/test_carry_over_order.py b/tests/core/test_carry_over_order.py index 0fc88939f..a144fd78f 100644 --- a/tests/core/test_carry_over_order.py +++ b/tests/core/test_carry_over_order.py @@ -145,6 +145,27 @@ def test_an_input_ending_after_the_last_date_carries(system, stored, asked): ) +@pytest.mark.parametrize("cache_only", [False, True]) +def test_an_input_ending_after_the_last_date_drops_overlapping_defaults( + system, cache_only +): + built = simulation(system, {}) + name = "day_input_without_helper" + holder = built.get_holder(name) + requested = periods.period("9999-12-31") + holder._do_not_store = cache_only + np.testing.assert_array_equal(built.calculate(name, requested), [0, 0]) + assert (name, requested) in built._fast_cache + assert holder._memory_storage.has(requested) == (not cache_only) + holder._do_not_store = False + + built.set_input(name, "day:9999-12-30:3", [10, 20]) + + assert holder.get_array(requested) is None + assert (name, requested) not in built._fast_cache + np.testing.assert_array_equal(built.calculate(name, requested), [10, 20]) + + @pytest.mark.parametrize("longer_first", [True, False], ids=["longer", "shorter"]) def test_an_input_ending_after_the_last_date_ends_last(system, longer_first): values = {"day:2012-01-01:4000000": [10, 20], "day:2012-01-01:2": [1, 2]} diff --git a/tests/core/test_own_period_input_overlapping_cache.py b/tests/core/test_own_period_input_overlapping_cache.py new file mode 100644 index 000000000..0fea5632f --- /dev/null +++ b/tests/core/test_own_period_input_overlapping_cache.py @@ -0,0 +1,104 @@ +"""An input for one of a variable's own periods updates its sums and twelfths. + +``calculate`` adds up a monthly flow over a year and divides a yearly flow +into a month, and caches the result at that period. An input set for one +month (or one year) of the variable replaced the value stored there but left +those results in place, so a later ``calculate`` returned the old sum or +twelfth (policyengine-core#579). The result depended on whether the longer +(or shorter) period had been read before the input was set. + +``set_input`` now drops what the simulation calculated for the variable over +periods that overlap the input's, by the rule the ``set_input`` helpers +follow (``test_set_input_helper_order.py``). Inputs are kept. +``test_set_input_helper_order_property.py`` compares such reads with a +simulation that calculated nothing first. +""" + +from __future__ import annotations + +import numpy as np +import pytest + +from tests.fixtures.set_input_helper_order import ( + BRANCH_NAME, + build_simulation, + read, +) + + +def _sequence(simulation, warm): + """Issue #579's table: read the other period or not, set, read again.""" + if warm: + read(simulation, "flow_m", "2013") + read(simulation, "flow_y", "2013-05") + simulation.set_input("flow_m", "2013-05", np.array([50.0, 0.0])) + simulation.set_input("flow_y", "2013", np.array([1200.0, 0.0])) + return read(simulation, "flow_m", "2013"), read(simulation, "flow_y", "2013-05") + + +@pytest.mark.parametrize("on_disk", [False, True]) +def test_sum_and_twelfth_follow_an_input_set_after_them(on_disk): + cold = _sequence(build_simulation(on_disk=on_disk), warm=False) + warm = _sequence(build_simulation(on_disk=on_disk), warm=True) + + for result in (cold, warm): + np.testing.assert_array_equal(result[0], [50.0, 0.0]) + np.testing.assert_array_equal(result[1], [100.0, 0.0]) + + +def test_annual_sum_follows_a_january_input(): + # The audit witness: a warm annual flow of 0, then January set to 1. + simulation = build_simulation() + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [0.0, 0.0]) + + simulation.set_input("flow_m", "2013-01", np.array([1.0, 1.0])) + + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [1.0, 1.0]) + + +def test_inputs_for_other_months_are_kept(): + simulation = build_simulation() + simulation.set_input("flow_m", "2013-04", np.array([7.0, 0.0])) + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [7.0, 0.0]) + + simulation.set_input("flow_m", "2013-05", np.array([50.0, 0.0])) + + np.testing.assert_array_equal(read(simulation, "flow_m", "2013-04"), [7.0, 0.0]) + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [57.0, 0.0]) + assert ("flow_m", "default", "2013-04") in { + (name, branch, str(period)) + for name, branch, period in simulation._user_input_keys + } + + +def test_sum_over_two_years_follows_an_input(): + simulation = build_simulation() + np.testing.assert_array_equal(read(simulation, "flow_m", "year:2013:2"), [0.0, 0.0]) + + simulation.set_input("flow_m", "2014-12", np.array([3.0, 0.0])) + + np.testing.assert_array_equal(read(simulation, "flow_m", "year:2013:2"), [3.0, 0.0]) + + +def test_branch_input_updates_the_sum_the_branch_reads(): + # The branch reads the sum its parent calculated until it is dropped; + # the parent keeps its own sum, which its inputs still give. + simulation = build_simulation() + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [0.0, 0.0]) + branch = simulation.get_branch(BRANCH_NAME) + + branch.set_input("flow_m", "2013-05", np.array([50.0, 0.0])) + + np.testing.assert_array_equal(read(branch, "flow_m", "2013"), [50.0, 0.0]) + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [0.0, 0.0]) + + +def test_input_through_the_holder_updates_the_sum(): + # ``Simulation.set_input`` drops the month from ``calculate``'s fast + # cache; an input set on the holder has to as well. + simulation = build_simulation() + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [0.0, 0.0]) + + simulation.get_holder("flow_m").set_input("2013-05", np.array([50.0, 0.0])) + + np.testing.assert_array_equal(read(simulation, "flow_m", "2013"), [50.0, 0.0]) diff --git a/tests/core/test_set_input_helper_order_property.py b/tests/core/test_set_input_helper_order_property.py index 17afd435b..900e06bb3 100644 --- a/tests/core/test_set_input_helper_order_property.py +++ b/tests/core/test_set_input_helper_order_property.py @@ -17,11 +17,11 @@ - ``calculate_add`` and ``calculate_divide`` called directly, whose results are cached where a plain read does not return them (#571): the requests are plain ``calculate`` calls. -- An input set for one of the variable's own periods (one month, or one - year) after a sum or a twelfth of the variable was calculated: the helpers - are not involved and that sum stays cached (policyengine-core#579). The - periods of another size are compared only for variables given no such - input after the calculations. + +An input set for one of the variable's own periods (one month, or one year) +after a sum or a twelfth of the variable was calculated involves no helper, +but follows the same rule (policyengine-core#579), so the periods of another +size are compared for every variable. """ from __future__ import annotations @@ -158,14 +158,8 @@ def test_inputs_do_not_depend_on_what_was_calculated_before( assert outcomes == fresh_outcomes == expected assert input_record(calculated) == input_record(fresh) - # A sum or a twelfth calculated before an input for one of the - # variable's own periods stays cached (policyengine-core#579). - given_own_period_input = { - name for name, period, _ in later_inputs if period in OWN_PERIODS[name] - } - other_periods_of = [name for name in NAMES if name not in given_own_period_input] - values = read_all(calculated, other_periods_of=other_periods_of, on_disk=on_disk) - fresh_values = read_all(fresh, other_periods_of=other_periods_of, on_disk=on_disk) + values = read_all(calculated, on_disk=on_disk) + fresh_values = read_all(fresh, on_disk=on_disk) assert values.keys() == fresh_values.keys() for key in values: assert same_arrays(values[key], fresh_values[key]), ( diff --git a/tests/core/test_user_input_keys_soundness.py b/tests/core/test_user_input_keys_soundness.py index 9fad83d1e..898001b75 100644 --- a/tests/core/test_user_input_keys_soundness.py +++ b/tests/core/test_user_input_keys_soundness.py @@ -157,6 +157,10 @@ def test_memory_input_wins_over_a_conflicting_disk_cache(invalidate): holder.put_in_cache(np.array([99.0]), JANUARY) simulation.memory_config.max_memory_occupation_pc = 101 simulation.set_input("rent", JANUARY, [42.0]) + assert holder._disk_storage.get(JANUARY) is None + # Own-period cleanup removed the old cache; model a later conflicting + # disk cache so export and replay still exercise both tiers. + holder._disk_storage.put(np.array([99.0]), JANUARY) np.testing.assert_array_equal(holder._memory_storage.get(JANUARY), [42.0]) np.testing.assert_array_equal(holder._disk_storage.get(JANUARY), [99.0]) @@ -234,6 +238,10 @@ def test_deleted_memory_input_does_not_turn_a_disk_calculation_into_an_input(): assert _key("rent", JANUARY) not in simulation._user_input_keys simulation.memory_config.max_memory_occupation_pc = 101 simulation.set_input("rent", JANUARY, [500.0]) + assert holder._disk_storage.get(JANUARY) is None + # Keep a calculated disk value behind the input to test its provenance + # after the memory input is deleted. + holder._disk_storage.put(np.array([0.0]), JANUARY, derived=True) np.testing.assert_array_equal(holder._memory_storage.get(JANUARY), [500.0]) np.testing.assert_array_equal(holder._disk_storage.get(JANUARY), [0.0]) @@ -373,6 +381,10 @@ def test_tier_provenance_replays_the_same_inputs_as_a_fresh_simulation( holder.put_in_cache(np.array([float(disk_value)]), JANUARY) simulation.memory_config.max_memory_occupation_pc = 101 simulation.set_input("rent", JANUARY, [memory_value]) + if not disk_is_input: + assert holder._disk_storage.get(JANUARY) is None + # Preserve the generated conflicting-tier witness after cleanup. + holder._disk_storage.put(np.array([float(disk_value)]), JANUARY) if delete_memory: simulation.delete_arrays("rent", "2025")