Skip to content

Conversation

@mloubout
Copy link
Contributor

Test in corresponding pro PR

@mloubout mloubout force-pushed the buffering-multi-guards branch from 19bfb04 to e5dc2af Compare January 16, 2026 15:30
@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.98%. Comparing base (ca12d81) to head (1a18c27).

Files with missing lines Patch % Lines
devito/types/parallel.py 33.33% 1 Missing and 1 partial ⚠️
devito/ir/support/guards.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2826   +/-   ##
=======================================
  Coverage   78.98%   78.98%           
=======================================
  Files         248      248           
  Lines       50815    50824    +9     
  Branches     4391     4392    +1     
=======================================
+ Hits        40134    40143    +9     
- Misses       9879     9880    +1     
+ Partials      802      801    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if c.scope.reads.get(self.f):
return c
return None
if c.scope.reads.get(self.f) and c.guards not in first_c:
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks very fragile at first glance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so? This just split the first read and last write per guard so that each different condition is respected

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, you don't need the extra "and c.guards ..."

just do

first_c.setdefault(c.guards) = c

and, for homogeneity, could you call it just mapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you do need the and, only the first cluster with the guard encountered is kept if you don't have the hand it'll replace it with the next one.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's what the setdefault in my snippet is for -- to avoid that extra guard

if c.scope.writes.get(self.f):
return c
return None
if c.scope.writes.get(self.f) and c.guards not in last_c:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

basically same comments above apply

@mloubout mloubout force-pushed the buffering-multi-guards branch from e5dc2af to 93a78f7 Compare January 16, 2026 19:36
func = vmax if self.is_forward_buffering else vmin
return func(*[Vector(i) for i in self.indices])[0]
for c in self.lastwrite:
idx = func(*[Vector(i) for i in extract_indices(self.f, self.dim, [c])])[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding a method that wraps extract_indices to tidy this section up?

Copy link
Contributor

Choose a reason for hiding this comment

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

or at least hoist the extract_indices bit to the line above to improve readability

if isinstance(npthreads, NPThreads):
npthreads = kwargs.get(npthreads.name, npthreads.size)
for th in search(npthreads, NPThreads):
npthreads = npthreads._subs(th, kwargs.get(th.name, th.size))
Copy link
Contributor

Choose a reason for hiding this comment

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

why subs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be an expression, e.g nthreads0 + nthreads1 +.... And need to replace all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be an expression, e.g nthreads0 + nthreads1 +.... And need to replace all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be an expression, e.g nthreads0 + nthreads1 +.... And need to replace all

Copy link
Contributor

Choose a reason for hiding this comment

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

mh, what would the reproducer be?

# If a symbolic object, it must be resolved
if isinstance(npthreads, NPThreads):
npthreads = kwargs.get(npthreads.name, npthreads.size)
for th in search(npthreads, NPThreads):
Copy link
Contributor

Choose a reason for hiding this comment

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

why search?

* `time-1` in the case of `foo(u[time-1], u[time], u[time+1])`
with a backwards-propagating `time` Dimension.
"""
last_idxs = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

mapper or retval for homogeneity (ultra-nitpicking)

@mloubout mloubout force-pushed the buffering-multi-guards branch from 93a78f7 to 1a18c27 Compare January 20, 2026 04:15
idx = func(*[Vector(i) for i in indices])[0]
mapper[c] = idx

return mapper
Copy link
Contributor

Choose a reason for hiding this comment

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

technically if you return a frozendict it'd be better but no big deal as long as we don't do crazy things at the caller site

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants