Centre stencil #17
Centre stencil #17
Conversation
devito/petsc/solve.py
Outdated
| the input equation (first argument). | ||
|
|
||
| NOTE: At the point of entry, the time derivatives are likely evaluated, but | ||
| not the spatial derivatives. This necessitates evaluating 'eqn' |
There was a problem hiding this comment.
you cannot pre-evaluate an expression -- thus destroying its (e.g.) IndexDerivative objects -- to obtain this information... assuming I'm reading this docstring correctly. You'd be hurting compilation elsewhere.
Can you add an example of what you're trying to accomplish here? Also, what's the use case?
There was a problem hiding this comment.
So looks like this only locally evaluate it to extract the middle part so it shouldn't impact the rest of the compiler.
evaluate can be a little bit expensive though might wanna add a TODO that needs to be improved somehow
devito/petsc/solve.py
Outdated
| @@ -7,6 +7,7 @@ | |||
| from devito.petsc.types import PETScArray, LinearSolveExpr, MatVecEq, RHSEq | |||
|
|
|||
| from sympy import simplify | |||
There was a problem hiding this comment.
Nitpick: line break between sympy/numpy/etc imports and Devito imports. Also group Devito imports
devito/petsc/solve.py
Outdated
| b, F_target = separate_eqn(eq, target) | ||
|
|
||
| # Args were updated so need to update target to enable uxreplace on F_target | ||
| new_target = {func for func in retrieve_functions(F_target) if |
There was a problem hiding this comment.
Nitpick: f rather than func might be tidier
devito/petsc/solve.py
Outdated
|
|
||
| # Args were updated so need to update target to enable uxreplace on F_target | ||
| new_target = {func for func in retrieve_functions(F_target) if | ||
| func.function == target.function}.pop() |
There was a problem hiding this comment.
May be safer to do this in two steps? i.e.
new_target = {f for f in retrieve_functions(F_target) if f.function == target.function}
assert len(new_target) == 1 # Sanity check: one target expected
new_target = new_target.pop()
That way if you ever end up with multiple targets for whatever reason, it will error, rather than failing cryptically
devito/petsc/solve.py
Outdated
| @@ -66,8 +75,9 @@ def separate_eqn(eqn, target): | |||
| zeroed_eqn = Eq(eqn.lhs - eqn.rhs, 0) | |||
| tmp = eval_time_derivatives(zeroed_eqn.lhs) | |||
| b = remove_target(tmp, target) | |||
| F_target = simplify(tmp - b) | |||
|
|
|||
| # Is this ok? Is there another way of using simplify | |||
There was a problem hiding this comment.
You can use devito.finite_differences.differentiable.diffify to turn sp.Add etc into dv.Add
There was a problem hiding this comment.
why do you need simplify, what's wrong with just tmp - b ?
There was a problem hiding this comment.
In some cases this does not work and it evaluates to -term + term , but simplify evaluates it to 0
|
|
||
|
|
||
| @extract_centre.register(Mul) | ||
| def _(expr, target): |
There was a problem hiding this comment.
Could this be consolidated with the Add/EvalDerivative dispatch? Seems like the only difference is the if not a.has(target)
tests/test_petsc.py
Outdated
|
|
||
|
|
||
| @skipif('petsc') | ||
| def test_centre_stencil(): |
There was a problem hiding this comment.
This should be done with pytest.mark.parametrize rather than hardcoding a bunch of values in a single test
There was a problem hiding this comment.
Also need to test something like (f * g.dx).dy to mimic div(grad) eqs.
| @@ -105,3 +115,52 @@ def _(expr, target): | |||
| @remove_target.register(Derivative) | |||
| def _(expr, target): | |||
| return 0 if expr.has(target) else expr | |||
|
|
|||
|
|
|||
| def centre_stencil(expr, target): | |||
There was a problem hiding this comment.
This function seems redundant?
There was a problem hiding this comment.
I just haven't used it yet but I will need the functionality
There was a problem hiding this comment.
I guess it will do more stuff later?
There was a problem hiding this comment.
The function won't do more, it will be used elsewhere
There was a problem hiding this comment.
Oh sorry I see what you mean, I removed the expr.evaluate so yes it is now redundant
ZoeLeibowitz
force-pushed
the
separate-eqn
branch
from
83de7e8
to
cd911cc
Compare
ZoeLeibowitz
force-pushed
the
centre-stencil
branch
from
e1f8cee
to
c3c1308
Compare
devito/petsc/solve.py
Outdated
| if not a.has(target): | ||
| args.append(a) | ||
| else: | ||
| a1 = centre_stencil(a, target) |
There was a problem hiding this comment.
just args.append(centre_stencil(a, target))
Add function to extract the centre stencil from an equation.