compiler: Move derivative.evaluate #1872
Conversation
| @@ -284,4 +284,7 @@ def indices_weights_to_fd(expr, dim, inds, weights, matvec=1): | |||
|
|
|||
| deriv = EvalDerivative(*terms, base=expr) | |||
|
|
|||
| # Evaluate remaining part of expression | |||
| deriv = deriv.evaluate | |||
There was a problem hiding this comment.
AS I said this is dangerous because now you can prevent shifts cancelling out and by forcing the "avg" to be evaluated this can lead to quite worse accuracy. This evaluate should always be the most last part in case there is half-node indices left.
There was a problem hiding this comment.
there are two issues here:
-
there's no failing test in the test suite capturing the behavior you're describing. Could you add one? Pushing to this PR would be completely fine
This evaluate should always be the most last part in case there is half-node indices left.
I'm not sure I see how my change would alter the order? The return deriv below would return control to derivative.py._eval_fd, so... what am I missing? perhaps with the MFE I'll see it
There was a problem hiding this comment.
my brain is not working very well these days, it doesn't change anything to have moved it.
There was a problem hiding this comment.
OK, no problem
Codecov Report
@@ Coverage Diff @@
## master #1872 +/- ##
=======================================
Coverage 89.55% 89.55%
=======================================
Files 210 210
Lines 35020 35038 +18
Branches 5291 5295 +4
=======================================
+ Hits 31362 31380 +18
Misses 3163 3163
Partials 495 495
Continue to review full report at Codecov.
|
| # Pure number | ||
| pass | ||
|
|
||
| terms.append(term) |
There was a problem hiding this comment.
you could probably postponn the *c here as well, may be slightly more efficient (one less Mul built and evaluated). This also prevents the full travers of EvalDerivative so good catch
There was a problem hiding this comment.
I tried, but I'm getting (expected) unevaluation errors:
self = <test_derivatives.TestFD object at 0x109a5dfa0>
def test_fd_new_lo(self):
grid = Grid((10,))
x = grid.dimensions[0]
u = Function(name="u", grid=grid, space_order=2)
dplus = "-1.0*u(x)/h_x + u(x + 1.0*h_x)/h_x"
dminus = "u(x)/h_x - 1.0*u(x - 1.0*h_x)/h_x"
> assert str(u.dx(x0=x + .5 * x.spacing).evaluate) == dplus
E AssertionError: assert '-1.0*u(x)/h_... 1.0*h_x)/h_x' == '-1.0*u(x)/h_... 1.0*h_x)/h_x'
E - -1.0*u(x)/h_x + u(x + 1.0*h_x)/h_x
E + -1.0*u(x)/h_x + 1.0*u(x + 1.0*h_x)/h_x
E ? ++++
test_derivatives.py:368: AssertionError
There was a problem hiding this comment.
Looks like it's introducing a weird 1.0* for some reason sympy being annoying again
This is in preparation for the "derivative evaluation overhaul" (DEO from now on!) that we've talked about recently.
-- A glimpse of the future --
I will need to introduce an
evaluate_partialtoDerivative(I cannot touchevaluatebecause we don't want API-level surprises). Suchevaluate_partial(better name needed) will expand theDerivativeto an intermediate high-level object, for exampleRepresenting the dot product between
fand an array of coefficients, that is, a "rolled" Derivative. This will be the object used by Devito throughout compilation and in particular throughout (and until) Cluster specialization.So, back to this PR... moving that
.evaluatewill allow me to share the first two steps ofDerivative.evaluatewithDerivative.evaluate_partialby moving them to a common function, or perhaps better, by havingDerivative.evaluatecallingDerivative.evaluate_partialfirst. Not sure yet, still very much work in progress, and I'm doing this only part time