devitocodes / devito

DSL and compiler framework for automated finite-differences and stencil computation
http://www.devitoproject.org
MIT License
557 stars 225 forks source link

api: revamp custom coefficients API #2434

Closed mloubout closed 1 week ago

mloubout commented 2 months ago

Superseeds #1644

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

Left to do for future iterations:

review-notebook-app[bot] commented 2 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

codecov[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mloubout commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

We should also check that:

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

works correctly

mloubout commented 1 month ago

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 commented 1 month ago

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