-
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
Refine CFL condition in seismic #1348
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1348 +/- ##
==========================================
+ Coverage 86.62% 86.76% +0.13%
==========================================
Files 183 183
Lines 26330 26379 +49
Branches 3619 3617 -2
==========================================
+ Hits 22808 22887 +79
+ Misses 3095 3065 -30
Partials 427 427
Continue to review full report at Codecov.
|
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 suggest adding somewhere the source of the changes.(i.e. the paper).
Seems fine for me. Tested locally.
It's ref to in |
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 can see tests passing, all seem fine for me, minor comments.
ea87c97
to
a92f081
Compare
eaa6d78
to
783fdce
Compare
@@ -29,7 +29,7 @@ def __init__(self, model, geometry, space_order=4, **kwargs): | |||
self.geometry = geometry | |||
|
|||
self.space_order = space_order | |||
self.dt = self.model.critical_dt | |||
self.dt = self.model.dtype(.9 * self.model.critical_dt) |
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.
Not sure I like this:
- We are seeing instabilities without this factor?
- Why this way instead of modifying the critical dt calc?
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.
- yes. You can look at Thorbecke's implementation where he basically says that with Q, it's not always stable and need to play with dt.
- I can try to put in model, but that would probably make it smaller for some other examples too.
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.
- Yeah, had seen that before.
- Indeed, it's a bit tricky - I'm not sure if it's better to have our 'default'
critical_dt
air on the side of caution for 'robustness' or not? What do you think? But maybe we could add afactor
kwarg
which is by default 0.9 or 1.0 (don't really mind)? At least this way the api will robust/trivial to change for any tinkering down the line - and then people will easily be able to look at the docstring to understand why such factors are sometimes needed. But I think that some_factor*critical_dt with no comment should be avoided at least.
a382547
to
306c212
Compare
refine elastic cfl according to Sei paper
https://doi.org/10.1137/0916052