API: revamp cross derivative shortcuts #2458

mloubout · 2024-09-24T18:44:06Z

No description provided.

mloubout · 2024-09-24T18:44:41Z

devito/finite_differences/tools.py

    def diff_f(expr, deriv_order, dims, fd_order, side=None, **kwargs):
-        return Derivative(expr, *as_tuple(dims), deriv_order=deriv_order,
-                          fd_order=fd_order, side=side, **kwargs)
+        # Spearate dimension to always have cross derivatives return nested


@FabioLuporini this basically does u.dxdy -> u.dx.dy

yeah, you could added it to the comment too (e.g., u.dxdy -> u.dx.dy)

mloubout · 2024-09-24T18:45:15Z

devito/ir/equations/algorithms.py

                indices = [j.xreplace(dimension_map) for j in indices]

-            mapper[i] = f.indexed[indices]
+            # Handle Array


@FabioLuporini There is probably a cleaner way but don't have time to spend more on this rn

codecov · 2024-09-24T19:12:01Z

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.01%. Comparing base (20a9de8) to head (db7a10a).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
devito/finite_differences/derivative.py	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2458   +/-   ##
=======================================
  Coverage   87.01%   87.01%           
=======================================
  Files         239      239           
  Lines       44958    44995   +37     
  Branches     8390     8399    +9     
=======================================
+ Hits        39118    39153   +35     
- Misses       5108     5109    +1     
- Partials      732      733    +1

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

FabioLuporini · 2024-09-25T07:21:52Z

devito/ir/equations/algorithms.py

+            if isinstance(f, Array) and f.initvalue is not None:
+                initv = [_lower_exprs(i, subs) for i in f.initvalue]
+                # TODO: fix rebuild to avoid new name
+                f = f._rebuild(name='%si' % f.name, initvalue=initv)


why do you need a new name? does it work if u instead pass function=None ?

Without a name you get caugth into "same name and indices, return the same object"

FabioLuporini · 2024-09-25T07:22:04Z

devito/ir/equations/algorithms.py

-            mapper[i] = f.indexed[indices]
+            # Handle Array
+            if isinstance(f, Array) and f.initvalue is not None:
+                initv = [_lower_exprs(i, subs) for i in f.initvalue]


can call it 'initvalue'

FabioLuporini · 2024-09-25T07:23:05Z

devito/finite_differences/derivative.py

+        weights = weights if weights is not None else self._weights
+
        x0 = self._process_x0(self.dims, x0=x0)
        _x0 = frozendict({**self.x0, **x0})


do we actually need the underscore for all these variable names ?

it'd get way less verbose and then easier to read without the initial underscore

For some yes but will try to cleanup a bit

FabioLuporini · 2024-09-25T07:24:08Z

devito/finite_differences/derivative.py

-            raise TypeError("Multi-dimensional Derivative, input expected as a dict")
+            raise TypeError("fd_order incompatible with dimensions")
+
+        # In case this was called on a cross derivative we need to propagate


"... on a perfect cross derivative (e.g. u.dxdy), so we need to ... " ?

actually this comment belongs to the inside of the if

FabioLuporini · 2024-09-25T07:25:32Z

devito/finite_differences/derivative.py

+        # In case this was called on a cross derivative we need to propagate
+        # the call to the nested derivative
+        if isinstance(self.expr, Derivative):
+            _fd_orders = {k: v for k, v in _fd_order.items() if k in self.expr.dims}


do we need small utility functions for these ? I have a feeling, but I may well be wrong, this is not the only place we have to perform this kind of information retrieval

FabioLuporini · 2024-09-25T07:26:39Z

devito/finite_differences/derivative.py


+        # Resolve nested derivatives
+        dsubs = {k: v for k, v in subs.items() if isinstance(k, Derivative)}
+        new_expr = self.expr.xreplace(dsubs)


nitpicking alert:
expr = ... would be more homogeneous than new_expr =

EdCaunt

Looking good, also fixes some of the aspects of Derivative that have been bugging me

devito/finite_differences/derivative.py

EdCaunt · 2024-09-25T08:50:29Z

devito/finite_differences/differentiable.py

-    prio = lambda x: getattr(x, '_fd_priority', 0)
+    # We want to get the object with highest priority
+    # We also need to make sure that the object with the largest
+    # set of dimensions is used when multiple ones with the same


Nitpick: this comment could be made clearer by replacing "ones" with "objects"

EdCaunt · 2024-09-25T08:55:16Z

devito/finite_differences/tools.py

    def diff_f(expr, deriv_order, dims, fd_order, side=None, **kwargs):
-        return Derivative(expr, *as_tuple(dims), deriv_order=deriv_order,
-                          fd_order=fd_order, side=side, **kwargs)
+        # Spearate dimension to always have cross derivatives return nested


Typo: "Spearate" -> "Separate", "dimension" -> "dimensions"

EdCaunt · 2024-09-25T09:03:09Z

tests/test_derivatives.py

+        grid = Grid((11, 11))
+        f = Function(name="f", grid=grid, space_order=2)
+
+        assert f.dxdy == f.dx.dy


Would be good to test that chaining derivatives with various x0 leads to sensible consolidation too.

https://github.com/devitocodes/devito/pull/2458/files#diff-f3c30bdc08b83267e30f815ee9e9a8e1ff22ad2b694efc6813652af0f2557869R395

…ortcuts

FabioLuporini

Left a few final comments, but also approved the PR!
Up to you if wanna handle them now or not or later on

Critical fixup and also a very nice clean up!

Many thanks

FabioLuporini · 2024-09-26T07:16:54Z

devito/finite_differences/derivative.py

        ret.append(self.expr.staggered or (None,))
        return tuple(ret)

+    def filter_dims(self, col, as_tuple=False, neg=False):


this could be a private method, but obv doable in another PR

FabioLuporini · 2024-09-26T07:19:55Z

devito/finite_differences/derivative.py


+    def filter_dims(self, col, as_tuple=False, neg=False):
+        """
+        Filter collectiion to only keep the derivative's dimensions as keys.


typo collection

also, "derivative" -> "Derivative"

again, fixable in abother branch

FabioLuporini · 2024-09-26T07:24:19Z

devito/finite_differences/tools.py

+        fd_order = as_tuple(fd_order)
+        deriv = Derivative(expr, dims[0], deriv_order=deriv_order[0],
+                           fd_order=fd_order[0], side=side, **kwargs)
+        for (d, do, fo) in zip(dims[1:], deriv_order[1:], fd_order[1:]):


these lines -- aka:

for (d, do, fo) in zip(dims, deriv_order, fd_order): expr = Derivative(expr ...) return expr

IOW, I don't think you need to separate out the first Derivative call?

FabioLuporini · 2024-09-26T07:25:33Z

devito/ir/equations/algorithms.py

+            if isinstance(f, Array) and f.initvalue is not None:
+                initvalue = [_lower_exprs(i, subs) for i in f.initvalue]
+                # TODO: fix rebuild to avoid new name
+                f = f._rebuild(name='%si' % f.name, initvalue=initvalue)


I'm not sure where's my previous comment gone honestly, I mean the one I thought I had written yesterday! Perhaps I forgot to append it in the end...

anyway, I was asking: instead of renaming, how about you just pass function=None?

how about you just pass function=Non

That's what I did first but somehow it led to some issues I'll think about it another time

mloubout added bug-py API api (symbolics, types, ...) labels Sep 24, 2024

mloubout commented Sep 24, 2024

View reviewed changes

mloubout force-pushed the revamp-cross branch from ce1c71e to 2c51dfe Compare September 24, 2024 18:51

mloubout force-pushed the revamp-cross branch 4 times, most recently from d54831c to c7360e2 Compare September 25, 2024 02:23

FabioLuporini reviewed Sep 25, 2024

View reviewed changes

EdCaunt reviewed Sep 25, 2024

View reviewed changes

mloubout force-pushed the revamp-cross branch from ef76cc4 to 263e354 Compare September 25, 2024 17:42

mloubout added 4 commits September 25, 2024 14:49

api: revamp cross derivatives to always be nested derivatives with sh…

4f8e86c

…ortcuts

api: avoid premature weight index substitution

8eac5bd

compiler: fix handling of initvalue

866f659

tests: fix unevalution tests with new cross deriv

41aaad8

mloubout force-pushed the revamp-cross branch from 263e354 to e712401 Compare September 25, 2024 18:49

FabioLuporini approved these changes Sep 26, 2024

View reviewed changes

misc: cleanup test and fix comments

a8226eb

mloubout force-pushed the revamp-cross branch from e712401 to a8226eb Compare September 26, 2024 11:53

api: make sure legacy symbolic with no coeffs defaults to taylor

db7a10a

mloubout merged commit 25d87fc into master Sep 26, 2024

mloubout deleted the revamp-cross branch September 26, 2024 14:18

API: revamp cross derivative shortcuts #2458

API: revamp cross derivative shortcuts #2458

Uh oh!

Conversation

mloubout commented Sep 24, 2024

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!

codecov bot commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

EdCaunt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 24, 2024 •

edited

Loading