From fd2fa0e4112cc2da902603195107dbbf8c70db80 Mon Sep 17 00:00:00 2001 From: georgebisbas Date: Sun, 9 Aug 2026 01:35:27 +0200 Subject: [PATCH] compiler: Fix issue #2235 -- default time_M with save-mode TimeFunction Cluster.dspace computes, per Function, an IntervalGroup of access offsets (parts), then unions them into one global Interval per Dimension. A regular (non-save) TimeFunction's modulo/stepping dimension (e.g. t) gets promote()'d onto its parent (time), reusing its offsets unchanged -- e.g. t's [0,1] becomes time[0,1], even though that "+1" is only ever safe because t wraps around its own circular buffer and says nothing about the raw time axis. When a save-mode TimeFunction is also present -- genuinely, exactly defined over time -- the union blindly combines the two, and the harmless promoted offset inflates the save-mode Function's exact bound. That inflated upper bound then flows into Dimension._arg_values, which subtracts it from the save-mode Function's own (already correct) time_M candidate, producing a value one too small. Fix: compute a second union restricted to Functions that natively define the Dimension (i.e. it's genuinely one of their own `.dimensions`, not merely reached via promotion), and use it to override just the upper bound of any Dimension that has such a native definer. The lower bound is left untouched: Dimension._arg_values only ever tightens it for a genuinely negative offset (`min(interval.lower, 0)`), which a promoted contribution satisfies correctly regardless of promotion -- confirmed by testing symmetric treatment first, which broke the existing test_indirection (dspace[time].lower shifted from 0 to 1 there because that scenario's non-native contribution is exactly the one supplying the correct lower bound). No new methods added to Interval/IntervalGroup; the fix is entirely local to Cluster.dspace using only union/indexing/construction already used elsewhere in this file. Adds test_default_timeM_with_saved (tests/test_dimension.py) covering the reported reproducer end to end (default time_M value and apply() correctness). Verified: tests/test_dimension.py + test_operator.py + test_ir.py (429 passed, including test_indirection), test_adjoint.py (74 passed), and a broader sweep across test_dse/test_buffering/test_checkpointing/ test_lower_clusters/test_lower_exprs/test_dle/test_visitors/ test_subdomains/test_interpolation (1004 passed, 2 pre-existing xfailed). 1507 tests passed total, 0 failures. --- devito/ir/clusters/cluster.py | 32 ++++++++++++++++++++++++++++++++ tests/test_dimension.py | 22 ++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/devito/ir/clusters/cluster.py b/devito/ir/clusters/cluster.py index c4b98ca4f9..65e79193e4 100644 --- a/devito/ir/clusters/cluster.py +++ b/devito/ir/clusters/cluster.py @@ -411,6 +411,38 @@ def dspace(self): # Dimension-centric view of the data space intervals = IntervalGroup.generate('union', *parts.values()) + # A SubIterator such as a ModuloDimension has offsets that are only + # meaningful relative to its own bounded, circular iteration (e.g., + # `t -> t+1` always safely wraps around a 2-slot buffer). `promote` + # (below) reinterprets such offsets, unchanged, against the parent + # Dimension (e.g. `t -> time`) so that e.g. halo/OOB computations + # elsewhere still see a `time`-keyed Interval. But when some *other* + # Function is natively -- and exactly -- defined over that same + # parent Dimension (e.g., a `save`-mode TimeFunction, whose data + # space along `time` is precisely its own declared shape), unioning + # in a merely-promoted upper offset incorrectly inflates the + # native Function's bound, which then propagates into e.g. the + # default `time_M` (too small by the promoted offset -- issue + # #2235). The upper bound is therefore restricted to only the + # natively-defined contributions whenever there is at least one. + # The lower bound doesn't need the same treatment: `_arg_values` + # only ever tightens it for a genuinely negative offset + # (`min(interval.lower, 0)`), which a promoted SubIterator + # contributes correctly regardless of the promotion + natives = {f: IntervalGroup([i for i in v if i.dim in f.dimensions], + relations=v.relations, mode=v.mode) + for f, v in parts.items()} + natives = {f: v for f, v in natives.items() if v} + if natives: + native_intervals = IntervalGroup.generate('union', *natives.values()) + rebuilt = [ + Interval(i.dim, i.lower, native_intervals[i.dim].upper, i.stamp) + if i.dim in native_intervals else i + for i in intervals + ] + intervals = IntervalGroup(rebuilt, relations=intervals.relations, + mode=intervals.mode) + # E.g., `db0 -> time`, but `xi NOT-> x` intervals = intervals.promote(lambda d: not d.is_Sub) intervals = intervals.zero(set(intervals.dimensions) - oobs) diff --git a/tests/test_dimension.py b/tests/test_dimension.py index 96bec12935..1ff7e8b118 100644 --- a/tests/test_dimension.py +++ b/tests/test_dimension.py @@ -257,6 +257,28 @@ def test_buffer1_direction(self, direction): for tree in trees: assert tree[0].direction == direction + def test_default_timeM_with_saved(self): + """ + MFE for issue #2235: the default `time_M`, when a `save`-mode + TimeFunction is mixed with a regular (modulo-buffered) one, must + come from the `save`-mode Function's own exact bound, not be + further shrunk by the other Function's harmless (because + circular) `+1` stepping offset. + """ + grid = Grid(shape=(4, 4)) + + u = TimeFunction(name='u', grid=grid) + usave = TimeFunction(name='usave', grid=grid, save=5) + + eqns = [Eq(u.forward, u + 1), + Eq(usave, u)] + + op = Operator(eqns) + + assert op.arguments()['time_M'] == 4 + op.apply() + assert all(np.all(usave.data[i] == i) for i in range(5)) + class TestSubDimension: