-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
examples: Born approximation for TTI media #1555
Conversation
Note: the perturbation is only in the velocity in the Born approx. |
Codecov Report
@@ Coverage Diff @@
## master #1555 +/- ##
===========================================
- Coverage 86.30% 58.42% -27.89%
===========================================
Files 217 200 -17
Lines 31428 29921 -1507
Branches 4186 3983 -203
===========================================
- Hits 27125 17482 -9643
- Misses 3818 11849 +8031
- Partials 485 590 +105
Continue to review full report at Codecov.
|
examples/seismic/tti/operators.py
Outdated
m = model.m | ||
vp = model.vp | ||
|
||
time_order = 1 if kernel == 'staggered' else 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't handle time_order = 2
right? should maybe catch as an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, I thought this was a first order op, first time I really looked ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the "linearization test". For me the proof of correctness is passing the linearization and adjoint tests.
Line 147 in 4a52d5b
def test_gradientFWI(self, dtype, shape, kernel, space_order, checkpointing): |
I guess for completeness could ensure the operator replicates acoustic modeling when isotropic. Not sure if people feel that is important or now, but it is like that acoustic op is nailed down via the accuracy notebook, so we can test that other ops reproduce acoustic isotropic wavefields.
examples/seismic/tti/operators.py
Outdated
m = model.m | ||
vp = model.vp | ||
|
||
time_order = 1 if kernel == 'staggered' else 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, I thought this was a first order op, first time I really looked ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reparametrize in term of squared slowness m
not velocity like for the rest of the operators and to match the examples and test setups.
I added that test as suggested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I wrong, or it needs a rebase on latest master for easier review ?
Thanks Oscar! Hope you are doing well!
721f4ad
to
12ad366
Compare
Hi George, all good, thx. You are correct, I made a mess. I hope that everything is in order now. |
12ad366
to
4f9543a
Compare
examples/seismic/tti/wavesolver.py
Outdated
The time-constant Azimuth angle (radians). | ||
""" | ||
if kernel != 'centered': | ||
raise RuntimeError('Only centered kernel is supported for the jacobian') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argument error not runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueError right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it also was resolved.
tests/test_gradient.py
Outdated
(np.float64, 4, 'OT2', (70, 80), True, setup), | ||
(np.float64, 4, 'OT2', (70, 80), False, setup), | ||
(np.float64, 4, 'centered', (70, 80), True, tti_setup), | ||
(np.float64, 4, 'centered', (70, 80), False, tti_setup), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no float32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add np.float32
. I just tried it and the test failed (AssertionError
) for both Checkpointing=True and False
even for the first order error
Operator `initdamp` run in 0.01 s
Operator `pad_vp` run in 0.01 s
Operator `pad_epsilon` run in 0.01 s
Operator `pad_delta` run in 0.01 s
Operator `pad_theta` run in 0.01 s
Operator `smoother` run in 0.01 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.03 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.03 s
1st order error, Phi(m0+dm)-Phi(m0): [ 0.87556505 -5.82210223]
2nd order error, Phi(m0+dm)-Phi(m0) - <J(m0)^T \delta d, dm>: [ 0.87554379 -5.82207074]
FOperator `initdamp` run in 0.01 s
Operator `pad_vp` run in 0.01 s
Operator `pad_epsilon` run in 0.01 s
Operator `pad_delta` run in 0.01 s
Operator `pad_theta` run in 0.01 s
Operator `smoother` run in 0.01 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `GradientTTI` run in 0.03 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
Operator `ForwardTTI` run in 0.02 s
1st order error, Phi(m0+dm)-Phi(m0): [ 0.87556505 -5.82210223]
2nd order error, Phi(m0+dm)-Phi(m0) - <J(m0)^T \delta d, dm>: [ 0.87554379 -5.82207074]
I don't really know what happened as with np.float64
the test runs without problems
Operator `initdamp` run in 0.01 s
Operator `pad_vp` run in 0.01 s
Operator `pad_epsilon` run in 0.01 s
Operator `pad_delta` run in 0.01 s
Operator `pad_theta` run in 0.01 s
Operator `smoother` run in 0.01 s
Operator `ForwardTTI` run in 0.03 s
Operator `ForwardTTI` run in 0.04 s
Operator `ForwardTTI` run in 0.12 s
Operator `ForwardTTI` run in 0.07 s
Operator `ForwardTTI` run in 0.07 s
Operator `ForwardTTI` run in 0.07 s
Operator `ForwardTTI` run in 0.06 s
Operator `ForwardTTI` run in 0.06 s
Operator `ForwardTTI` run in 0.07 s
1st order error, Phi(m0+dm)-Phi(m0): [ 0.92561436 -10.858962 ]
2nd order error, Phi(m0+dm)-Phi(m0) - <J(m0)^T \delta d, dm>: [ 1.97902976 -10.94126684]
.Operator `initdamp` run in 0.01 s
Operator `pad_vp` run in 0.01 s
Operator `pad_epsilon` run in 0.01 s
Operator `pad_delta` run in 0.01 s
Operator `pad_theta` run in 0.01 s
Operator `smoother` run in 0.01 s
Operator `ForwardTTI` run in 0.03 s
Operator `ForwardTTI` run in 0.05 s
Operator `GradientTTI` run in 0.05 s
Operator `ForwardTTI` run in 0.05 s
Operator `ForwardTTI` run in 0.05 s
Operator `ForwardTTI` run in 0.06 s
Operator `ForwardTTI` run in 0.06 s
Operator `ForwardTTI` run in 0.07 s
Operator `ForwardTTI` run in 0.07 s
Operator `ForwardTTI` run in 0.07 s
1st order error, Phi(m0+dm)-Phi(m0): [ 0.92561436 -10.858962 ]
2nd order error, Phi(m0+dm)-Phi(m0) - <J(m0)^T \delta d, dm>: [ 1.97902976 -10.94126684]
I'll have to look it up and that might take a while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new commit with the test for np.float32
and several tests from CI failed. I review the code in operators.py
and wavesolver.py
and I couldn't find an error. Then, I tried without the optimization flags (-O3 and -ffast-math
) but the test still fails. @mloubout if you have any suggestions or comments about this, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is definitely something off, I would understand if things were a bit off but that's way off. Can you plot the convergence curves and such to see how it behaves and maybe make sure model/data looks ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discovered what the problem was. The tti_setup
function uses a default value of tn=250.0
, so only the direct wave is registered, doubling the time solves the problem, thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see why the bump to 500... OK. Instead of changing it from tti_setup
and run
, can you pass the value from the tests only? also, is there a smaller value such that the forward wave is recorded, such as 300 or 350? or a smaller grid? we need to bring down the turnaround time of these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reduced the grid dimensions as well as the recording time to 400.
tests/test_gradient.py
Outdated
(np.float64, 4, 'centered', (70, 80), True, tti_setup), | ||
(np.float64, 4, 'centered', (70, 80), False, tti_setup), | ||
]) | ||
def test_gradientFWI(self, dtype, space_order, kernel, shape, ckp, setup_func): | ||
r""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems so, and now I realize there are others in that file, thx.
8cefa00
to
29c0b43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a > 5 minutes increase in turnaround time in each build . Can we reduce this. Options incluse:
- Bringing back that tn=500 to tn=250 should already have an impact, no?
- Dropping either some of the so=4 or some of the so=8
- Dropping some of the double precision?
examples/seismic/tti/tti_example.py
Outdated
@@ -21,7 +21,7 @@ def tti_setup(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=250.0, | |||
return AnisotropicWaveSolver(model, geometry, space_order=space_order, **kwargs) | |||
|
|||
|
|||
def run(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=250.0, | |||
def run(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=500.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this bump to 500. I don't see why it's necessary. I'm used to the turnaround time of python tti_example.py
which I find not too short not too long, doubling it would make it slow for no evident reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tn
was set again to be 250.0. It only is changed in the specific test as you suggested.
examples/seismic/tti/tti_example.py
Outdated
@@ -7,7 +7,7 @@ | |||
from examples.seismic.tti import AnisotropicWaveSolver | |||
|
|||
|
|||
def tti_setup(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=250.0, | |||
def tti_setup(shape=(50, 50, 50), spacing=(20.0, 20.0, 20.0), tn=500.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like this bump to 500. See comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tn
was set again to be 250.0. It only is changed in the specific test as you suggested.
6165b83
to
cc0c465
Compare
…ror if kernel != centered
…nd minimize CI times
cc0c465
to
2de2bfe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Also approved offline by @mloubout
Thanks, merged. |
At this point @FabioLuporini might be asking: where is @ofmla? xD well, I've been extra-busy with some other projects and preparing classes, but here is a small contribution. This PR adds new operators to the anisotropic solver. In the absence of Mathias @jkwashbourne could take a look.