FD for composite staggered expr #1247

mloubout · 2020-04-23T14:10:54Z

Fixes handling of expression with multiple Function and multiple staggering.

Add are split so that FD handles each staggering properly
Mul are "evaluated" at highest priority staggering before FD so that the expression has single indices

Important note. I had to make a choice on the priority levels of each staggering that is
param < not staggered, NODE < staggered

This example from slack now produce same output for bot operators

import numpy as np
from devito import *

grid = Grid(shape=(5,5))

v = VectorTimeFunction(name="v", grid=grid, time_order=1, space_order=4)
p1 = TimeFunction(name="p1", grid=grid, time_order=1, space_order=4, staggered=NODE)
p2 = TimeFunction(name="p2", grid=grid, time_order=1, space_order=4, staggered=NODE)

v[0].data[:] = 5.
v[1].data[:] = 5.

den = np.zeros((5,5), dtype=np.float32)
den[:] = 2.

A = Function(name="A", grid=grid, space_order=4, staggred=NODE)

initialize_function(A, 1./den, 0, dtype=np.float32)

av = VectorTimeFunction(name="av", grid=grid, time_order=1, space_order=4)

test =  div(A*v).args[0]._eval_at(p2).evaluate 

eq1 = Eq(av, A*v)
eq2 = Eq(p1, div(av))

op = Operator([eq1, eq2])
op.apply(time_M=0)

eq3 = Eq(p2, div(A*v))
op2 = Operator([eq3])
op2.apply(time_M=0)

print(p1.data)
print(p2.data)

fixes #1248

devito/finite_differences/derivative.py

devito/finite_differences/differentiable.py

codecov · 2020-04-23T14:36:35Z

Codecov Report

Merging #1247 into master will increase coverage by 0.06%.
The diff coverage is 96.40%.

@@            Coverage Diff             @@
##           master    #1247      +/-   ##
==========================================
+ Coverage   86.16%   86.23%   +0.06%     
==========================================
  Files         177      177              
  Lines       25182    25311     +129     
  Branches     3546     3561      +15     
==========================================
+ Hits        21699    21826     +127     
  Misses       3061     3061              
- Partials      422      424       +2

Impacted Files	Coverage Δ
devito/types/caching.py	`94.44% <ø> (ø)`
devito/finite_differences/differentiable.py	`91.63% <87.50%> (-0.35%)`	⬇️
devito/finite_differences/derivative.py	`88.11% <100.00%> (+0.99%)`	⬆️
devito/finite_differences/finite_difference.py	`95.91% <100.00%> (-0.09%)`	⬇️
devito/symbolics/manipulation.py	`88.52% <100.00%> (+0.32%)`	⬆️
devito/types/basic.py	`94.75% <100.00%> (+0.07%)`	⬆️
devito/types/dense.py	`93.78% <100.00%> (+0.09%)`	⬆️
tests/test_derivatives.py	`100.00% <100.00%> (ø)`
tests/test_staggered_utils.py	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e918e26...31ba38d. Read the comment docs.

FabioLuporini

also, this should be added to some (new?) userapi notebook ?

devito/finite_differences/derivative.py

mloubout · 2020-04-27T13:18:11Z

@FabioLuporini discovered some deeper issue I have to work on so lots of that may change. Maybe we want to put a small PR for Diff objects with single function in it first (one line change)

devito/finite_differences/derivative.py

devito/finite_differences/differentiable.py

devito/symbolics/manipulation.py

devito/types/basic.py

devito/finite_differences/derivative.py

devito/finite_differences/differentiable.py

rhodrin · 2020-04-30T19:49:15Z

tests/test_derivatives.py

@@ -421,7 +421,7 @@ def test_fd_adjoint(self, so, ndim, derivative, adjoint_name):
        deriv = getattr(f, derivative)
        coeff = 1 if derivative == 'dx2' else -1
        expected = coeff * getattr(f, derivative).evaluate.subs({x.spacing: -x.spacing})
-        assert deriv.T.evaluate == expected
+        assert simplify(deriv.T.evaluate) == simplify(expected)


Why is simplify now needed?

Because now subs is ours fully here and creates (1/_h_x) * f(x) instead of f(x)/h_x so doesn match without simplify (compiler refactorize so no flop increase issue)

so is it missing a SymPy evaluation somewhere?

I could make it evaluate but that's be quite expensive for more complex expressions and not necessary.

rhodrin · 2020-04-30T19:52:53Z

tests/test_staggered_utils.py

@@ -59,3 +76,87 @@ def test_is_param(ndim):
        for dd in d:
            avg = .5 * (avg + avg.subs({dd: dd - dd.spacing}))
        assert f2._eval_at(var).evaluate == avg
+
+
+@pytest.mark.parametrize('expr, expected', [


Worth adding a more intricate composite expression also?

as I wrote above, yes, and perhaps x-fail it if it needs to

rhodrin · 2020-04-30T19:54:11Z

Looks good. Some nitpicking above.

devito/finite_differences/derivative.py

devito/finite_differences/differentiable.py

devito/symbolics/manipulation.py

devito/types/basic.py

FabioLuporini · 2020-05-01T07:25:44Z

tests/test_staggered_utils.py

@@ -59,3 +76,87 @@ def test_is_param(ndim):
        for dd in d:
            avg = .5 * (avg + avg.subs({dd: dd - dd.spacing}))
        assert f2._eval_at(var).evaluate == avg
+
+
+@pytest.mark.parametrize('expr, expected', [


as I wrote above, yes, and perhaps x-fail it if it needs to

FabioLuporini · 2020-05-01T11:59:26Z

tests/test_derivatives.py

@@ -421,7 +421,7 @@ def test_fd_adjoint(self, so, ndim, derivative, adjoint_name):
        deriv = getattr(f, derivative)
        coeff = 1 if derivative == 'dx2' else -1
        expected = coeff * getattr(f, derivative).evaluate.subs({x.spacing: -x.spacing})
-        assert deriv.T.evaluate == expected
+        assert simplify(deriv.T.evaluate) == simplify(expected)


so is it missing a SymPy evaluation somewhere?

devito/types/dense.py

FabioLuporini

so I still don't understand the dichotomy Add vs Mul, and I'd like to clarify that, potentially offline if that makes it quicker.

the PR is approved though

devito/finite_differences/derivative.py

fd: mul bug fix for Pow and derivs fd: handle pow

FabioLuporini

There are stylistic things I don't like (too many new functions/methods used in only one place), but they cannot be a stopper. So for me this is GTG.

mloubout requested review from FabioLuporini and rhodrin April 23, 2020 14:10

FabioLuporini reviewed Apr 23, 2020

View reviewed changes

devito/finite_differences/derivative.py Show resolved Hide resolved

devito/finite_differences/derivative.py Outdated Show resolved Hide resolved

devito/finite_differences/differentiable.py Outdated Show resolved Hide resolved

mloubout force-pushed the ind-ref-diff branch from 1928564 to d5b92fb Compare April 23, 2020 14:33

FabioLuporini reviewed Apr 23, 2020

View reviewed changes

mloubout force-pushed the ind-ref-diff branch 4 times, most recently from 59dd033 to 1d4d7c3 Compare April 23, 2020 16:46

FabioLuporini reviewed Apr 27, 2020

View reviewed changes

mloubout force-pushed the ind-ref-diff branch 14 times, most recently from 91d93ad to 1077067 Compare April 28, 2020 11:23

mloubout added better code bug-py labels Apr 28, 2020

FabioLuporini reviewed Apr 30, 2020

View reviewed changes

mloubout force-pushed the ind-ref-diff branch 2 times, most recently from 6f47540 to f519444 Compare April 30, 2020 12:57

rhodrin reviewed Apr 30, 2020

View reviewed changes

devito/finite_differences/derivative.py Outdated Show resolved Hide resolved

rhodrin reviewed Apr 30, 2020

View reviewed changes

devito/finite_differences/derivative.py Outdated Show resolved Hide resolved

rhodrin reviewed Apr 30, 2020

View reviewed changes

devito/finite_differences/derivative.py Outdated Show resolved Hide resolved

rhodrin reviewed Apr 30, 2020

View reviewed changes

devito/finite_differences/differentiable.py Show resolved Hide resolved

rhodrin reviewed Apr 30, 2020

View reviewed changes

FabioLuporini reviewed May 1, 2020

View reviewed changes

mloubout force-pushed the ind-ref-diff branch 2 times, most recently from 4551021 to c6fe52d Compare May 1, 2020 11:45

FabioLuporini approved these changes May 1, 2020

View reviewed changes

mloubout force-pushed the ind-ref-diff branch 3 times, most recently from 2ec990b to 085f232 Compare May 1, 2020 14:51

FabioLuporini approved these changes May 4, 2020

View reviewed changes

devito/finite_differences/derivative.py Show resolved Hide resolved

mloubout added 6 commits May 6, 2020 08:46

fd: fix single function in differentiable with stggering

06acbf6

fd: add preprocessign of args for FD on Mul

1039d21

fd: mul bug fix for Pow and derivs fd: handle pow

types: rework sub to keep track on/off grid

17a3d5b

test: add test for staggered expr and staggered expr derivs

ce990b1

types: on grid from indices

92c93f5

fix comments

e454cd7

mloubout force-pushed the ind-ref-diff branch from a577045 to bd1bee8 Compare May 6, 2020 12:47

derivative: add explanatory comments of eval_at split

31ba38d

mloubout force-pushed the ind-ref-diff branch from bd1bee8 to 31ba38d Compare May 6, 2020 12:50

FabioLuporini approved these changes May 6, 2020

View reviewed changes

mloubout merged commit 6966dfb into master May 6, 2020

mloubout deleted the ind-ref-diff branch May 6, 2020 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FD for composite staggered expr #1247

FD for composite staggered expr #1247

mloubout commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

FabioLuporini left a comment

mloubout commented Apr 27, 2020

rhodrin Apr 30, 2020

mloubout Apr 30, 2020

FabioLuporini May 1, 2020

mloubout May 1, 2020

rhodrin Apr 30, 2020 •

edited

Loading

FabioLuporini May 1, 2020

mloubout May 1, 2020

rhodrin commented Apr 30, 2020

FabioLuporini May 1, 2020

FabioLuporini May 1, 2020

FabioLuporini left a comment

FabioLuporini left a comment

FD for composite staggered expr #1247

FD for composite staggered expr #1247

Conversation

mloubout commented Apr 23, 2020 • edited Loading

codecov bot commented Apr 23, 2020 • edited Loading

Codecov Report

FabioLuporini left a comment

Choose a reason for hiding this comment

mloubout commented Apr 27, 2020

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

rhodrin Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhodrin commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

mloubout commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

rhodrin Apr 30, 2020 •

edited

Loading