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.60140% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.99%. Comparing base (cec0542) to head (958720a).

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%   86.99%   -0.07%     
==========================================
  Files         238      238              
  Lines       45171    44939     -232     
  Branches     8417     8387      -30     
==========================================
- Hits        39326    39096     -230     
  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

EdCaunt · 2024-08-02T08:45:22Z

devito/finite_differences/tools.py

@@ -305,6 283,12 @@ def generate_indices(expr, dim, order, side=None, matvec=None, x0=None):
 else:
 o_min -= 1

+ if nweights > 0 and (o_max - o_min 1) != nweights:
+ assert nweights == (o_max - o_min 1) 1


Explanatory comment on this check would probably be worthwhile

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


EdCaunt · 2024-09-11T08:33:39Z

devito/builtins/initializers.py

@@ -145,9 145,9 @@ def create_gaussian_weights(sigma, lw):
 weights = [w/w.sum() for w in (np.exp(-0.5/s**2*(np.linspace(-l, l, 2*l 1))**2)
 for s, l in zip(sigma, lw))]
 processed = []
- for w in weights:
+ for (w, l) in zip(weights, lw):


lw is a pretty cryptic variable name. I would use a more descriptive one or add a comment

EdCaunt · 2024-09-11T08:36:23Z

devito/builtins/initializers.py

 temp = list(w)
- while len(temp) < 2*max(lw) 1:
+ while len(temp) < 2*l 1:


Is this for padding? If so, why not use np.pad? Would be more tidy and robust.

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?

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

devito/ir/equations/equation.py

@@ -119,6 121,22 @@ def detect(cls, expr):
 OpMin = Operation('min')


+identity_mapper = {


Should this mapper be frozen?

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

devito/types/equation.py

@@ -65,14 63,43 @@ class Eq(sympy.Eq, Evaluable):

 def __new__(cls, lhs, rhs=0, subdomain=None, coefficients=None, implicit_dims=None,
 **kwargs):
+ if coefficients is not None:


Probably only want to have this warning in one place, otherwise it will print two or three warnings rather than just one. We should probably also have a warning saying that coefficients='symbolic' can be removed when instantiating a Function, if there isn't one already.

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

devito/types/equation.py

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


"compatibility"

…nges

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

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

EdCaunt Sep 11, 2024

EdCaunt Sep 11, 2024

EdCaunt Sep 11, 2024

EdCaunt Sep 11, 2024

EdCaunt Sep 11, 2024

		@@ -119,6 121,22 @@ def detect(cls, expr):
		OpMin = Operation('min')


		identity_mapper = {

api: revamp custom coefficients API #2434

Are you sure you want to change the base?

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

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

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