compiler: compute sparse position/floor in fp64 to fix off-by-one cell shift by mloubout · Pull Request #2992 · devitocodes/devito

mloubout · 2026-08-05T18:15:58Z

Precompute indices ones when using coordinates (bit cheaper for lots of recs)
Upcast to double for indices compuation to avoid fp32 truncation errors shifting the result.

The lifiting is implemented as a Cire pass this way it'll catch generic floor/... as well

codecov · 2026-08-05T18:32:32Z

Codecov Report

❌ Patch coverage is 29.72973% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.35%. Comparing base (110463b) to head (915bc6d).

Files with missing lines	Patch %	Lines
tests/test_interpolation.py	12.50%	14 Missing ⚠️
devito/symbolics/inspection.py	0.00%	3 Missing and 1 partial ⚠️
tests/test_dse.py	0.00%	4 Missing ⚠️
devito/ir/cgen/printer.py	50.00%	1 Missing and 1 partial ⚠️
devito/types/sparse.py	77.77%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (110463b) and HEAD (915bc6d). Click for more details.

HEAD has 12 uploads less than BASE

Flag BASE (110463b) HEAD (915bc6d)

18 10

pytest-gpu-gcc- 6 4

pytest-gpu-icx- 3 2

pytest-gpu-nvc-nvidiaX 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2992       +/-   ##
===========================================
- Coverage   83.54%   52.35%   -31.20%     
===========================================
  Files         257      257               
  Lines       53922    53952       +30     
  Branches     4613     4619        +6     
===========================================
- Hits        45050    28245    -16805     
- Misses       8074    24704    +16630     
- Partials      798     1003      +205

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`68.54% <52.94%> (-0.02%)`	⬇️
pytest-gpu-gcc-	`43.27% <29.72%> (-34.91%)`	⬇️
pytest-gpu-icx-	`43.12% <29.72%> (-34.99%)`	⬇️
pytest-gpu-nvc-nvidiaX	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

review-notebook-app · 2026-08-05T19:28:49Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini

I think the main question that I have is: why not inlining DOUBLE(...) casts directly when building the interpolation operation? why is this implemented via hoisting into fp64 scalars?

FabioLuporini · 2026-08-06T07:55:25Z

        yield self._do_generate(exprs, exclude, cbk_search)


+def _is_floor(e):


this is quite hacky, let the search look for the specific type if it really needs to be that specific

FabioLuporini · 2026-08-06T07:58:47Z


+    # Predicate on Cluster used to pick which ones this pass fires on.
+    # Subclasses override to target a different kind of cluster.
+    _cluster_filter = staticmethod(lambda c: c.is_dense)


why don't we always do it with all floors (and maybe not just that), be it dense or sparse ?

so why do we need this _cluster_filter at all?

Foir the standard pass that is for dense cluster

For easier extension/specialization in the future

I'm not sure agree. If you restrict it to dense clusters, for example, divisions won't be hoisted anymore in sparse clusters. And imho it doesn't make much sense that the base CireInvariants class restricts to dense; if anything, it should be selected subclasses

This isn't new only lifted. It always have filtered to dense but can change it if you want but it's gonna change probably a lot of tests

FabioLuporini · 2026-08-06T08:00:09Z

+                    issubclass(k.dtype, np.integer)):
+                continue
+            inner = k.base
+            if _is_floor(inner) and (v.free_symbols & aliaseds):


instead of just floors, we could name this class CireInvariantGeometry or something like that, and have it target all step-wise math operations that are so accuracy-sensitive

you could have for example a _types class attribute, and use it here

FabioLuporini · 2026-08-07T12:15:49Z

+    out of an inner loop is a strict win regardless of the cluster kind.
+    """
+
+    _cluster_filter = staticmethod(lambda c: True)


same here... not sure we need it

FabioLuporini · 2026-08-09T09:56:13Z

                else:
-                    # E.g., `x0_blk0` or (`a[y_m+1]` => `y not in imapper`)
+                    # E.g., `x0_blk0` (outer to writeto, must stay in ispace
+                    # so later passes -- `Lift`, `Fuse` -- see it)


it's not that "so later passes see it", it's that w/o it, it'd be wrong

FabioLuporini · 2026-08-09T10:05:51Z


+    # Predicate on Cluster used to pick which ones this pass fires on.
+    # Subclasses override to target a different kind of cluster.
+    _cluster_filter = staticmethod(lambda c: c.is_dense)


I'm not sure agree. If you restrict it to dense clusters, for example, divisions won't be hoisted anymore in sparse clusters. And imho it doesn't make much sense that the base CireInvariants class restricts to dense; if anything, it should be selected subclasses

FabioLuporini · 2026-08-09T10:06:18Z

    """

-    PAR_CHUNK_NONAFFINE = 3
+    PAR_CHUNK_NONAFFINE = 1


why, this is an important change

FabioLuporini · 2026-08-09T10:13:12Z

+        assert summary1[('section0', None)].ops == 9
+        assert summary1[('section1', None)].ops == 9
+        assert summary1[('section2', None)].ops == 31
+        assert summary1[('section3', None)].ops == 46


that's a lot of code changes...

mloubout added the compiler label Aug 5, 2026

mloubout force-pushed the cire-int-invariants branch from 24006cd to 04411cd Compare August 5, 2026 19:28

mloubout force-pushed the cire-int-invariants branch 4 times, most recently from 1ad6fd3 to 038d26b Compare August 6, 2026 02:46

FabioLuporini reviewed Aug 6, 2026

View reviewed changes

mloubout force-pushed the cire-int-invariants branch 4 times, most recently from da61a3e to 173da0d Compare August 7, 2026 11:58

FabioLuporini reviewed Aug 7, 2026

View reviewed changes

mloubout force-pushed the cire-int-invariants branch 5 times, most recently from a8550da to dd910c8 Compare August 8, 2026 22:40

FabioLuporini reviewed Aug 9, 2026

View reviewed changes

mloubout force-pushed the cire-int-invariants branch from 0e461ab to 74f833a Compare August 10, 2026 03:10

FabioLuporini approved these changes Aug 10, 2026

View reviewed changes

mloubout closed this Aug 10, 2026

mloubout force-pushed the cire-int-invariants branch from 915bc6d to 110463b Compare August 10, 2026 17:58

		yield self._do_generate(exprs, exclude, cbk_search)


		def _is_floor(e):

Uh oh!

Conversation

mloubout commented Aug 5, 2026

Uh oh!

codecov Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

review-notebook-app Bot commented Aug 5, 2026

Uh oh!

FabioLuporini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 5, 2026 •

edited

Loading