Modifications to rpt2_geoclaw #159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjleveque
added a commit
to rjleveque/geoclaw
that referenced
this pull request
Oct 1, 2020
Will be needed when clawpack/riemann#159 is merged.
The logic is now cleaner and some work is skipped in dry cells so it is faster. Relative to earlier tests on this branch, this goes back to the original approach of putting fluctuations in bmasdq if s<0 and into bpasdq if s>0 without splitting if s near 0. But it now skips the splitting entirely only if eta < min(topo1,topo3) rather than max(..) and creates transverse fluctuation going into the cell if eta < topo, which seems more correct but could cause changes in results.
|
I can indeed confirm that this improves behavior of the storm surge examples both removing much of the instability in shallow waters away from the storm and allowing for us to safely increase to second order. I think this then says it is safe to merge this and update all the tests. |
|
Additional tests by @mandli and myself have shown this gives results that are similar to past results and seem better when different, in particular this may eliminate some of the stability issues seen in the past for long runs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Various issues were found in trying to determine (with @mjberger) why radially-symmetric initial data does not lead to symmetric solutions (to machine accuracy). This PR has several modifications that might change the results obtained, so do not merge until it is more carefully tested and compared with previous results on some real-world problems.
Switch to using Jacobian from left or right state, not Roe average, in determining wave speeds and eigenvectors.
The original used
ql, qrto compute Roe averages, but since the transverse solver splits up a left-goingasdq(ifimp==1) or a right-goingasdq(ifimp==2) into up-going and down-going components, there is not good justification for using the state from the other side of the normal solve that generatedasdq. Now it uses the Jacobian matrix from the appropriate side.Ideally we would also use
qin the row of cells above or below, similar to what is done for variable-coefficient acoustics, but in that case the necessary info is in theauxarrays and we do passauxfrom above and below intorpt2asaux3andaux1respectively. But we do not pass inqfrom above and below.In addition, a bug in the second component of the eigenvectors was fixed. It should be the orthogonal component of velocity to the one that defines
s(2).Finally, when s(2) is close to zero this component of the flux difference is now split equally between bmasdq and bpasdq to improve symmetry. The cutoff is when
abs(s(2)) < 0.001 * sqrt(g*hlr)wherehlris nowhon the proper side. Not clear if this is the best tolerance.Note that indentation is screwy in this routine and should also be cleaned up, but for now I left alone to highlight the lines that changed more substantially.