api: revamp custom coefficients API #2434

mloubout · 2024-07-31T14:15:21Z

Superseeds #1644

Currently implemented with backward compatibility, we can drop it at some point.

Left to do for future iterations:

weights for cross-derivs .dxdy
weights with indice, i.e weights=(indices,weights) for fully custom stencils
?

review-notebook-app · 2024-07-31T14:15:26Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2024-07-31T14:41:38Z

Codecov Report

Attention: Patch coverage is 98.67550% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.00%. Comparing base (cec0542) to head (77e0021).
Report is 10 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2434      +/-   ##
==========================================
- Coverage   87.06%   87.00%   -0.07%     
==========================================
  Files         238      239       +1     
  Lines       45171    44947     -224     
  Branches     8417     8388      -29     
==========================================
- Hits        39326    39104     -222     
+ Misses       5112     5111       -1     
+ Partials      733      732       -1

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

FabioLuporini

This is great, thanks!

Couple of questions, mostly related to the aesthetics of the API

do we now risk a proliferation of f.d(weights=weights, ...) in the PDE specification? things might get a little noisy
- Is it useless or perhaps difficult to provide, in addition, an API such that if you do obj = Space(f, w, ...) then obj.dx == f.dx(weights=w) ?
I think, now that we're revamping the API, we need to add an alias such that f.dx(weights=w0) == f.dx(w=w0) , again to minimize verbosity
EvalDerivatives and IndexDerivatives are all constructed automatically and seamlessly regardless of whether taylor or custom coefficients are use, right?

mloubout · 2024-08-01T12:41:56Z

do we now risk a proliferation of f.d(weights=weights, ...) in the PDE specification?

Well yes, and no. One thing that will be added to tuto doc is that you can provide your own backend for FD through fd_weights_registry so if you gonna have your own coefficients everywhere you can create your own my_custom_weight and then add it to fd_weights_registry and then use it.

Not sure about adding additional wrapper object for this i don't think it's make it much cleaner.

we need to add an alias such that f.dx(weights=w0) == f.dx(w=w0)

Completely fine yes, will add it

EvalDerivatives and IndexDerivative

Yes, It actually makes everything quite simpler for the compiler, everything now goes through standard FD and creates EvalDerivatives and IndexDerivative. There is no more intricated post-process replacement pass after evaluation.

EdCaunt

Looks like a pretty big cleanup, much needed.

There should be a test to make sure that changing the weights of a Derivative changes the hash, and that two Derivatives with identical weights + spec hash the same. Also tests for pickling and unpickling derivatives with custom coefficients.

devito/finite_differences/derivative.py

devito/finite_differences/tools.py

EdCaunt · 2024-08-02T08:51:50Z

devito/types/equation.py

+    @classmethod
+    def _apply_coeffs(cls, expr, coefficients):
+        """
+        This process legacy API of Substitution/Coefficients applying the weights


"Processes"

devito/types/equation.py

EdCaunt · 2024-08-02T09:12:37Z

do we now risk a proliferation of f.d(weights=weights, ...) in the PDE specification?

The user can also do something like:

subs = {f.dx: f.dx(weights=weights0),
        f.dy: f.dy(weights=weights0),
        f.dx2: f.dx2(weights=weights1)}
eq0 = Eq(f, f.dx + f.dx2)
eq1 = Eq(f, f.dx + f.dy +1)
eqs = [eq.subs(subs) for eq in (eq0, eq1)]

We should probably have a test for this

EdCaunt · 2024-08-02T09:26:09Z

We should also check that:

solve(f.dx(weights=weights), f.forward)

works correctly

georgebisbas · 2024-08-02T13:50:03Z

examples/seismic/tutorials/07_DRP_schemes.ipynb

Do the big figure changes need to be committed?

Can you avoid committing them somehow?

is the image indeed different?
If not, one thing I do is 'git add -p'
https://nuclearsquid.com/writings/git-add/

examples/seismic/tutorials/07_DRP_schemes.ipynb

mloubout · 2024-08-07T12:55:57Z

eqs = [eq.subs(subs) for eq in (eq0, eq1)]

I would prefer to avoid adding these type of constructions that are not really good or clean

EdCaunt · 2024-08-12T11:46:07Z

Notebook needs the text reworking. It still references the old API throughout.

georgebisbas · 2024-09-10T15:05:31Z

devito/ir/clusters/algorithms.py

+                # Populate the Array (the "map" part)
+                processed.append(e.func(a.indexify(), rhs, operation=None))
+
+                # Set all untouched entried to the identity value if necessary


devito/builtins/initializers.py

EdCaunt · 2024-09-11T08:51:56Z

devito/finite_differences/tools.py

+        shape = weights.shape
+        return shape[weights.dimensions.index(wdim)], wdim
+    else:
+        return len(list(weights)), None


Why do you need the list() here?

perhaps weights could be a generator?

It can also be Weights which behave weirdly with len

devito/ir/equations/equation.py

devito/types/equation.py

EdCaunt · 2024-09-11T08:58:43Z

devito/types/equation.py

        kwargs['evaluate'] = False
+        # Backward compat


"compatibility"

…nges

FabioLuporini · 2024-09-22T09:59:16Z

devito/finite_differences/coefficients.py

-                raise ValueError("Number of FD weights provided does not "
-                                 "match the functions space_order")
-
+        warn("The Coefficient API is deprecated and will be removed, coefficients should"


This is creating one warning per Coefficient, another option is to register a callback to call at atexit(), or more simply use a global file-level flag to emit the warning only once ?

FabioLuporini · 2024-09-22T09:59:41Z

devito/finite_differences/coefficients.py

    def __init__(self, *args):
-
+        warn("The Coefficient API is deprecated and will be removed, coefficients should"


same as before

devito/finite_differences/tools.py

FabioLuporini · 2024-09-22T10:03:51Z

devito/finite_differences/tools.py

+    if weights is None:
+        return 0, None
+    elif isinstance(weights, Function):
+        wdim = {d for d in weights.dimensions if d not in expr.dimensions}


potential nitpicking and therefore ignorable:

is it worth introducing a SymbolicWeights subclass (can't call it just Weights because it'd be the same name as finite_differences/differentiable.py::Weights) and add a few properties such as .dimension ? or is this impossible because you need expr?

FabioLuporini · 2024-09-22T10:04:26Z

devito/finite_differences/tools.py

+        shape = weights.shape
+        return shape[weights.dimensions.index(wdim)], wdim
+    else:
+        return len(list(weights)), None


perhaps weights could be a generator?

devito/types/equation.py

FabioLuporini · 2024-09-22T10:08:00Z

devito/types/equation.py

        kwargs['evaluate'] = False
+        # Backward compat
+        rhs = cls._apply_coeffs(rhs, coefficients)
+        lhs = cls._apply_coeffs(lhs, coefficients)
        obj = sympy.Eq.__new__(cls, lhs, rhs, **kwargs)


nitpicking: blank lines above and below?

FabioLuporini · 2024-09-22T10:08:52Z

examples/seismic/model.py

@@ -356,7 +356,7 @@ def _cfl_coeff(self):
        if 'lam' in self._physical_parameters or 'vs' in self._physical_parameters:
            coeffs = fd_w(1, range(-self.space_order//2+1, self.space_order//2+1), .5)
            c_fd = sum(np.abs(coeffs[-1][-1])) / 2
-            return np.sqrt(self.dim) / self.dim / c_fd
+            return .95 * np.sqrt(self.dim) / self.dim / c_fd


so this justifies the change in expected norm values above?

Yes. The CFL bound is extremely tight without it so for some complicated models it can become unstable, this is safer

FabioLuporini · 2024-09-22T10:09:04Z

examples/seismic/viscoelastic/viscoelastic_example.py

@@ -41,8 +41,8 @@ def run(shape=(50, 50), spacing=(20.0, 20.0), tn=1000.0,
 @pytest.mark.parametrize("dtype", [np.float32, np.float64])
 def test_viscoelastic(dtype):
    _, _, _, [rec1, rec2, v, tau] = run(dtype=dtype)
-    assert np.isclose(norm(rec1), 12.30114, atol=1e-3, rtol=0)
-    assert np.isclose(norm(rec2), 0.312462, atol=1e-3, rtol=0)
+    assert np.isclose(norm(rec1), 12.62339, atol=1e-3, rtol=0)


same here as above

FabioLuporini · 2024-09-22T10:09:27Z

devito/finite_differences/derivative.py

 from devito.types.utils import DimensionTuple

 __all__ = ['Derivative']


-class Derivative(sympy.Derivative, Differentiable, Reconstructable):
+class Derivative(sympy.Derivative, Differentiable, Pickable):


why isn't Reconstructable enough ?

You only get sympy's rkwargs at pickle if you only use Reconstructable so you would loose all the x0/weights/side/transpose at pickle, quite weird it didn't trigger a bug before

I wonder whether it's due to this , which we might want to lift in Reconstructable. Because as for the rest a Pickable should reconstruct like a Reconstructable...

anyway, no need to investigate now

FabioLuporini · 2024-09-24T07:16:38Z

devito/finite_differences/derivative.py

 from devito.types.utils import DimensionTuple

 __all__ = ['Derivative']


-class Derivative(sympy.Derivative, Differentiable, Reconstructable):
+class Derivative(sympy.Derivative, Differentiable, Pickable):


I wonder whether it's due to this , which we might want to lift in Reconstructable. Because as for the rest a Pickable should reconstruct like a Reconstructable...

anyway, no need to investigate now

FabioLuporini · 2024-09-24T07:17:24Z

devito/finite_differences/coefficients.py

-                raise ValueError("Number of FD weights provided does not "
-                                 "match the functions space_order")
-
+        deprecations.coeff_warn


nitpicking: perhaps a memoized_meth would be neater because you're performing an action rather than retrieving a property

but really, this is super-nitpicking. Can revisit another day

mloubout added API api (symbolics, types, ...) feature-request labels Jul 31, 2024

mloubout mentioned this pull request Jul 31, 2024

dsl: Overhaul custom fd weights #1644

Closed

mloubout force-pushed the coeffs-revamp branch 2 times, most recently from c7ae7c4 to bc057cd Compare July 31, 2024 16:53

FabioLuporini reviewed Aug 1, 2024

View reviewed changes

EdCaunt reviewed Aug 2, 2024

View reviewed changes

georgebisbas reviewed Aug 2, 2024

View reviewed changes

mloubout commented Aug 2, 2024

View reviewed changes

examples/seismic/tutorials/07_DRP_schemes.ipynb Outdated Show resolved Hide resolved

mloubout force-pushed the coeffs-revamp branch from cbcbe95 to 9276723 Compare August 7, 2024 12:56

mloubout force-pushed the coeffs-revamp branch 2 times, most recently from cacb60d to b7e35b5 Compare August 21, 2024 20:17

mloubout force-pushed the coeffs-revamp branch from b7e35b5 to c407b03 Compare September 4, 2024 17:36

EdCaunt force-pushed the coeffs-revamp branch from 67b557c to 65b69e5 Compare September 9, 2024 14:15

mloubout force-pushed the coeffs-revamp branch from 99f692b to 58a6278 Compare September 9, 2024 15:45

georgebisbas reviewed Sep 10, 2024

View reviewed changes

EdCaunt reviewed Sep 11, 2024

View reviewed changes

mloubout force-pushed the coeffs-revamp branch from 58a6278 to c58e310 Compare September 12, 2024 14:46

mloubout and others added 5 commits September 12, 2024 13:40

api: revamp custom coefficients API to be passed to derivatives directly

e586281

misc: flake8

803404a

builtins: fix gaussian kernel for non-square

a553d5c

api: add w= for weights shortcut

d88bf3d

examples: Update custom coefficients notebook text to reflect API cha…

d6006ae

…nges

mloubout force-pushed the coeffs-revamp branch from c58e310 to 2dd0d4c Compare September 12, 2024 17:41

examples: avoid too tight cfl for elastic

20313d8

mloubout force-pushed the coeffs-revamp branch from 2dd0d4c to 20313d8 Compare September 12, 2024 17:57

mloubout added 2 commits September 12, 2024 18:52

builtins: remove small clutter

7c7994c

api: fix pickle with derivative

958720a

EdCaunt approved these changes Sep 16, 2024

View reviewed changes

FabioLuporini reviewed Sep 22, 2024

View reviewed changes

api: avoid duplicate deprecation warnings

77e0021

FabioLuporini approved these changes Sep 24, 2024

View reviewed changes

mloubout merged commit 4c8be71 into master Sep 24, 2024
30 of 31 checks passed

mloubout deleted the coeffs-revamp branch September 24, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: revamp custom coefficients API #2434

api: revamp custom coefficients API #2434

mloubout commented Jul 31, 2024 •

edited

Loading

review-notebook-app bot commented Jul 31, 2024

codecov bot commented Jul 31, 2024 •

edited

Loading

FabioLuporini left a comment

mloubout commented Aug 1, 2024 •

edited

Loading

EdCaunt left a comment

EdCaunt Aug 2, 2024

EdCaunt commented Aug 2, 2024 •

edited

Loading

EdCaunt commented Aug 2, 2024

georgebisbas Aug 2, 2024

EdCaunt Aug 5, 2024

georgebisbas Aug 25, 2024

mloubout commented Aug 7, 2024

EdCaunt commented Aug 12, 2024

georgebisbas Sep 10, 2024

EdCaunt Sep 11, 2024

FabioLuporini Sep 22, 2024

mloubout Sep 23, 2024

EdCaunt Sep 11, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

mloubout Sep 23, 2024

FabioLuporini Sep 22, 2024

FabioLuporini Sep 22, 2024

mloubout Sep 23, 2024

FabioLuporini Sep 24, 2024

FabioLuporini Sep 24, 2024

FabioLuporini Sep 24, 2024

		def __init__(self, *args):

		warn("The Coefficient API is deprecated and will be removed, coefficients should"

api: revamp custom coefficients API #2434

api: revamp custom coefficients API #2434

Conversation

mloubout commented Jul 31, 2024 • edited Loading

review-notebook-app bot commented Jul 31, 2024

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

FabioLuporini left a comment

Choose a reason for hiding this comment

mloubout commented Aug 1, 2024 • edited Loading

EdCaunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdCaunt commented Aug 2, 2024 • edited Loading

EdCaunt commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout commented Aug 7, 2024

EdCaunt commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout commented Jul 31, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading

mloubout commented Aug 1, 2024 •

edited

Loading

EdCaunt commented Aug 2, 2024 •

edited

Loading