api: fix staggered evaluation for add/mul #2782

mloubout · 2025-10-31T14:21:29Z

No description provided.

codecov · 2025-10-31T14:41:58Z

Codecov Report

❌ Patch coverage is 83.18584% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.01%. Comparing base (59c357b) to head (37af191).

Files with missing lines	Patch %	Lines
devito/passes/clusters/implicit.py	0.00%	10 Missing ⚠️
devito/types/dense.py	71.42%	3 Missing and 1 partial ⚠️
devito/finite_differences/differentiable.py	72.72%	2 Missing and 1 partial ⚠️
devito/passes/iet/parpragma.py	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
- Coverage   83.02%   83.01%   -0.01%     
==========================================
  Files         248      248              
  Lines       50804    50851      +47     
  Branches     4479     4484       +5     
==========================================
+ Hits        42180    42216      +36     
- Misses       7857     7866       +9     
- Partials      767      769       +2

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`68.50% <61.84%> (-0.04%)`	⬇️
pytest-gpu-nvc-nvidiaX	`69.02% <61.84%> (-0.03%)`	⬇️

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

☔ 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.

review-notebook-app · 2025-11-01T02:55:53Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini

Uncontroversial

JDBetteridge

A lot of the norms in the tests and notebooks have changed significantly, how do we know these are now the "right values". And is the test really valuable if we do not know what the true value is?

One could suggest that these are in some sense "regression tests", but then we definitely should not be changing the values!

mloubout · 2025-11-06T03:25:56Z

A lot of the norms in the tests and notebooks have changed significantly

The one norm that changed a lot in test_mpi.py .... was indeed because of a bug introduced, made me find it so thanks, the rest is small changes due to a few extra interpolation but within tolerance

FabioLuporini · 2025-11-06T08:12:24Z

devito/finite_differences/differentiable.py



 class DiffDerivative(IndexDerivative, DifferentiableOp):
    pass


drop pass now

FabioLuporini · 2025-11-06T08:13:03Z

devito/finite_differences/differentiable.py

 class DiffDerivative(IndexDerivative, DifferentiableOp):
    pass

+    def _eval_at(self, func):


isn't it the case of IndexDerivative too?

FabioLuporini · 2025-11-06T08:13:33Z

devito/finite_differences/differentiable.py

        kwargs.pop('is_commutative', None)
        return self.func(*args, **kwargs)

+    def _eval_at(self, func):


nitpicking: Perhaps could be moved into a mixin class from which all *Derivative class inherit

FabioLuporini · 2025-11-06T08:14:18Z

devito/types/basic.py

            # Two indices are aligned if they differ by an Integer*spacing.
-            v = (i - j)/d.spacing
+            if not i.has(d):
+                # Maybe a subdimension


nitpicking, SubDimension

FabioLuporini · 2025-11-06T08:14:30Z

devito/types/basic.py

-            v = (i - j)/d.spacing
+            if not i.has(d):
+                # Maybe a subdimension
+                dims = {sd for sd in i.free_symbols if getattr(sd, 'is_Dimension', False)


avoids circular (so local) import using getattr

it's just a hack to avoid a hack in that case?

anyway, u just reminded me that unless i is a Number or a Dimension w/o an offset, it will be of type AffineIndexAccessFunction https://github.com/devitocodes/devito/blob/main/devito/types/dimension.py#L1822

so all you have to here is

try: sds, = i.sds except (AttributeError, ValueError): # Expected exactly one StencilDimension continue

btw should we not raise a NotImplementedError if len(sds) > 1 instead?

Don't think it's better. You have to check all three of i.sds, i.d, o.ofs and see if one is a sbdimensions it doesn't make it cleaner or simpler.

btw should we not raise a NotImplementedError if len(sds) > 1 instead?

No if there is multiple dimensions it's considered that the user decided to specifiy its own indices and to be valid ones. Would lead to a lot of breakign cases to raise and error here

FabioLuporini · 2025-11-06T08:17:55Z

devito/types/basic.py

        for the compiler.
        """
+        mapper = self._grid_map
+        subs = mapper.pop('subs', {})


since _grid_map is (must be) a cached_property, use .get() instead of .pop()

In fact, _grid_map, now that you make me think twice about it, should return a frozendict to be pedantic

I should not be cached no, that would break all of the FD since f and f._subs(x, x+.5) would end up with the same _grid_map when the second needs interpolation

using pop so that can just iterate mapper below

oh that's because we still have that crazy hack that makes every instance of f(x,y,z) reference the same __dict__ as the "origin" Function?

No this is because this is based on indices. Every f.subs is the same f with different indices (and has too or it would get really nasty with data/.....) and this checks if those specific indices are on the grid given the reference index (staggered "dim")

FabioLuporini · 2025-11-06T08:18:33Z

devito/types/basic.py

        # Evaluate. Since we used `self.function` it will be on the grid when
        # evaluate is called again within FD
        retval = retval._evaluate(**kwargs)
+        if subs:


I don't think this if is needed, .subs should take O(1) when subs is empty by returning immediately

FabioLuporini · 2025-11-06T08:19:32Z

devito/types/dense.py

+            except KeyError:
+                pass
+
        if mapper:


nitpicking, but same as before. We can reduce verbosity by avoiding these if since they practically speaking won't improve performance

FabioLuporini · 2025-11-06T08:19:43Z

devito/types/dense.py

+
        if mapper:
            return self.subs(mapper)
        return self


nitpicking, perhaps a blank line

FabioLuporini · 2025-11-06T08:20:47Z

tests/test_derivatives.py

        eqne = eqn.evaluate.rhs
        assert simplify(eqne - (p._subs(y, yp).evaluate * f).dx(x0=xp).evaluate) == 0

+    def test_param_stagg_add(self):


quite a few changes have been made, and it seems weird that only one new test was required?

were they all fixing the same exact (set of) thing(s), which is entirely captured by this test? if so, OK

This is the only "change" that is tested. Everything else isadjustments from removing is_Parameter and using subs in eval_for_fd instead of explicit derivatives

JDBetteridge

Just flagging a couple of other big changes that might need investigation (or given they are just in the notebooks, maybe re-running after your bug fix)

JDBetteridge · 2025-11-06T12:15:08Z

examples/cfd/02_convection_nonlinear.ipynb

   "source": [
    "from devito import norm\n",
-    "assert np.isclose(norm(u), norm(U[0]), rtol=1e-5, atol=0)"
+    "assert np.isclose(norm(u), norm(U[0]), rtol=1e-2, atol=0)"


This is 3 orders of magnitude change in the difference between two values that are notionally the same

The numpy implementation is techinically wrong, as it does staggering but does not interpolate u/v where needed so there is some difference now because forcing the devito equation to be the same would be tricky since "incorrect"

JDBetteridge · 2025-11-06T12:15:39Z

examples/userapi/07_functions_on_subdomains.ipynb

   "outputs": [],
   "source": [
-    "assert np.isclose(np.linalg.norm(rec.data), 4263.511, atol=0, rtol=1e-4)"
+    "assert np.isclose(np.linalg.norm(rec.data), 3640.584, atol=0, rtol=1e-4)"


This jump is also pretty big!

Likely some parameters interpolation that were not being processed due to the SubDimensions. Will double check

Yes, basically comes from parameters (damp, ro, ...) being properly interpolated now.

…rtree at read

mloubout added the API api (symbolics, types, ...) label Oct 31, 2025

mloubout force-pushed the fd-eval-add branch from f709216 to be8bdbf Compare October 31, 2025 14:25

mloubout force-pushed the fd-eval-add branch 2 times, most recently from 766cc81 to a156d3c Compare October 31, 2025 16:05

mloubout force-pushed the fd-eval-add branch 2 times, most recently from 1a9951a to dc0e8f5 Compare November 3, 2025 13:01

FabioLuporini approved these changes Nov 3, 2025

View reviewed changes

mloubout force-pushed the fd-eval-add branch 7 times, most recently from 42588ae to 6c6d45f Compare November 4, 2025 04:17

JDBetteridge reviewed Nov 5, 2025

View reviewed changes

mloubout force-pushed the fd-eval-add branch from 6c6d45f to 45bc728 Compare November 6, 2025 03:24

FabioLuporini reviewed Nov 6, 2025

View reviewed changes

JDBetteridge requested changes Nov 6, 2025

View reviewed changes

mloubout force-pushed the fd-eval-add branch 5 times, most recently from cfcadc9 to ef1f7d8 Compare November 12, 2025 15:06

mloubout added 4 commits November 13, 2025 11:51

api: fix staggered evaluation for add/mul

6771d4c

api: remove obsolete and clunky is_parameter

f580a6e

examples: remove usage of obsolete paramter kw

a5da08b

examples: update notebooks

24c4cfb

api: prevent evaluated derivatives to be re-evaluted

2d32230

mloubout force-pushed the fd-eval-add branch 7 times, most recently from a8d5757 to 7a7fdd1 Compare November 18, 2025 14:34

compiler: prevent hosted per-thread arrays are dereferenced within pa…

c1544b8

…rtree at read

mloubout force-pushed the fd-eval-add branch from 7a7fdd1 to c1544b8 Compare November 18, 2025 16:08

compiler: patch multi-subdim isapce handling

37af191



		class DiffDerivative(IndexDerivative, DifferentiableOp):
		pass

api: fix staggered evaluation for add/mul #2782

Are you sure you want to change the base?

api: fix staggered evaluation for add/mul #2782

Uh oh!

Conversation

mloubout commented Oct 31, 2025

Uh oh!

codecov bot commented Oct 31, 2025 • 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 Nov 1, 2025

Uh oh!

FabioLuporini left a comment

Choose a reason for hiding this comment

Uh oh!

JDBetteridge left a comment

Choose a reason for hiding this comment

Uh oh!

mloubout commented Nov 6, 2025

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!

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!

JDBetteridge 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!

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!

4 participants

codecov bot commented Oct 31, 2025 •

edited

Loading