api: fix interp/eval of expressions by mloubout · Pull Request #2843 · devitocodes/devito

mloubout · 2026-02-04T18:50:59Z

No description provided.

codecov · 2026-02-04T19:10:49Z

Codecov Report

❌ Patch coverage is 97.40260% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.97%. Comparing base (676b8bf) to head (3fc5b91).

Files with missing lines	Patch %	Lines
devito/finite_differences/differentiable.py	96.07%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2843      +/-   ##
==========================================
- Coverage   78.97%   78.97%   -0.01%     
==========================================
  Files         248      248              
  Lines       50901    50936      +35     
  Branches     4395     4404       +9     
==========================================
+ Hits        40199    40226      +27     
- Misses       9904     9909       +5     
- Partials      798      801       +3

☔ 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 · 2026-02-05T01:53:08Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini

there's quite a few changes here, but not as many new tests covering them?

FabioLuporini · 2026-02-05T08:41:07Z

devito/finite_differences/differentiable.py

-                                            for i in self._args_diff)))
+        if not self._args_diff:
+            return DimensionTuple()
+        # Get indices of all args and merge them


nitpicking: blank line

FabioLuporini · 2026-02-05T08:41:11Z

devito/finite_differences/differentiable.py

+        for a in self._args_diff:
+            for d, i in a.indices.getters.items():
+                mapper.setdefault(d, []).append(i)
+        # Filter unique indices


nitpicking: blank line

FabioLuporini · 2026-02-05T08:41:15Z

devito/finite_differences/differentiable.py

+        # Filter unique indices
+        mapper = {k: v[0] if len(v) == 1 else tuple(filter_ordered(v))
+                  for k, v in mapper.items()}
+        return DimensionTuple(*mapper.values(), getters=tuple(mapper.keys()))


nitpicking: blank line

nitpicking: can just be getters=tuple(mapper)

devito/finite_differences/differentiable.py

FabioLuporini · 2026-02-05T08:44:30Z

devito/finite_differences/differentiable.py

    def dimensions(self):
-        return tuple(filter_ordered(flatten(getattr(i, 'dimensions', ())
-                                            for i in self._args_diff)))
+        if not self._args_diff:


why do we need these two lines? highest_priority(self).dimensions should already return it, or there's something non-consistent about it

Because a generic Differentiable does not necessarily has an _args_diff. For. example 1/h_x is a Differentiable but it doesn't have any argument that is Differentiable

but the property is still there, it's not that you are exposing yourself to an AttributeError... so (1/h_x)._args_diff will return the empty tuple and it should just be fine no?

An empty tuple doesn't have a .staggered or .dimension property. And on top of it (1/h_x)._args_diff will return 1/h_x which wiull be an infinite recursion

FabioLuporini · 2026-02-05T08:44:42Z

devito/finite_differences/differentiable.py

+
+    @cached_property
+    def staggered(self):
+        if not self._args_diff:


same story as before, I think

... unless I'm missing something

FabioLuporini · 2026-02-05T08:45:08Z

devito/finite_differences/differentiable.py

 def highest_priority(DiffOp):
+    if not DiffOp._args_diff:
+        return DiffOp
    # We want to get the object with highest priority


FabioLuporini · 2026-02-05T08:45:20Z

devito/finite_differences/differentiable.py

@@ -475,12 +487,19 @@ def has_free(self, *patterns):


 def highest_priority(DiffOp):


small letters diff_op as it's an instance

FabioLuporini · 2026-02-05T08:46:08Z

devito/finite_differences/differentiable.py


+@interp_for_fd.register(Mul)
+def _(expr, x0, **kwargs):
+    # For a expression (e.g Mul or Add), we interpolate the whole expression


"an expression"

so the comment says "Mul or Add" but the dispatcher only targets Mul. Is it expected?

EdCaunt · 2026-02-05T10:35:44Z

devito/finite_differences/differentiable.py

+        # Filter unique indices
+        mapper = {k: v[0] if len(v) == 1 else tuple(filter_ordered(v))
+                  for k, v in mapper.items()}
+        return DimensionTuple(*mapper.values(), getters=tuple(mapper.keys()))


nitpicking: can just be getters=tuple(mapper)

EdCaunt · 2026-02-05T10:47:27Z

devito/finite_differences/differentiable.py

+
+    # Split args between those that need interp and those that don't
+    def test0(a):
+        return all(a.indices[d] is i for d, i in x0.items() if d in a.dimensions)


nitpick: blank line

EdCaunt · 2026-02-05T10:48:51Z

devito/finite_differences/tools.py

-                f"'{expr}' must be of type Differentiable, not {type(expr)}"
-            ) from None
+        except Exception as e:
+            raise type(e)(f"Error while computing finite-difference for expr={expr}: "


Reporting the type of expr here would make this error message more useful

Not sure what you mean the message is raise type(e) ... that's type already

mloubout added the API api (symbolics, types, ...) label Feb 4, 2026

mloubout force-pushed the stagg-encore branch 3 times, most recently from f2e36fa to 171969e Compare February 5, 2026 00:55

mloubout force-pushed the stagg-encore branch 2 times, most recently from 018e9d3 to 10eac67 Compare February 5, 2026 02:42

FabioLuporini reviewed Feb 5, 2026

View reviewed changes

EdCaunt reviewed Feb 5, 2026

View reviewed changes

api: fix interp/eval of expressions

3fc5b91

mloubout force-pushed the stagg-encore branch from 10eac67 to 3fc5b91 Compare February 5, 2026 18:28

		@@ -475,12 +487,19 @@ def has_free(self, *patterns):


		def highest_priority(DiffOp):

Conversation

mloubout commented Feb 4, 2026

Uh oh!

codecov bot commented Feb 4, 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 Feb 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 4, 2026 •

edited

Loading