Skip to content

compiler: Fix issue #2235 #2996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
georgebisbas wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −0
Open

compiler: Fix issue #2235 #2996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions devito/ir/clusters/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@FabioLuporini FabioLuporini Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is where this comment starts becoming hard to understand
IMO, this whole comment could be replaced by a strightforward example, such as

# Handle inconsistencies due to nonsaved- and saved-TimeFunctions, if any
# E.g., given `f(t, x, y)`  and `fsave(time, x, y)`, ...
# ...

should be a 3~4 lines long comment with an example to stand the change of getting understood. Otherwise, a simple :

# Handle inconsistencies due to nonsaved- and saved-TimeFunctions, if any; see issue #2235

# 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}
Comment on lines +432 to +435

@FabioLuporini FabioLuporini Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely just

key = lambda ...
natives = {f: intervals.project(key) for f, intervals in parts.items()}

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)

@FabioLuporini FabioLuporini Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so I got to this point and I have a very simple question; if there are both nonsaved- and saved-TimeFunction, instead of this pretty complicated logic, why don't you simply filter the nonsaved TimeFunctions off parts before constructing the intervals? something along the lines of

from devito.tools import split
....
....
# <comment as above>
functions = {f for f in parts if f.is_TimeFunction}
nonsaved, saved = split(functions, lambda f: f.save is None)
if saved:
    parts = {f: v for f, v in parts.items() if f not in nonsaved}

# E.g., `db0 -> time`, but `xi NOT-> x`
intervals = intervals.promote(lambda d: not d.is_Sub)
intervals = intervals.zero(set(intervals.dimensions) - oobs)
Expand Down
22 changes: 22 additions & 0 deletions tests/test_dimension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down