Skip to content
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

flux_lax_friedrichs should be specialized for equations with discontinuous parameter-as-variables #1725

Closed
jlchan opened this issue Nov 10, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@jlchan
Copy link
Contributor

jlchan commented Nov 10, 2023

Right now, if you have a bottom topography or another spatially dependent parameter in your solution, you need to store it as an additional variable in the solution.

Note that rhs! should return zero for any variables which are parameters but not true variables. However, if this additional variable is discontinuous, and your surface flux is flux_lax_friedrichs, the LxF penalization will be non-zero and enforce continuity during the ODE solve.

I noticed this issue with the quasi-1D SWE equations, as well as an incoming PR for the quasi-1D Euler equations. I'm not sure if it as issue for SWE too.

@jlchan jlchan changed the title flux_lax_friedrichs should be specialized for equations with discontinuous parameter-as-variables (like SWE) flux_lax_friedrichs should be specialized for equations with discontinuous parameter-as-variables Nov 10, 2023
@andrewwinters5000
Copy link
Member

That is true. This was first noticed by Lars for the AcousticPerturbationEquations2D so he wrote a special version of the Lax-Friedrichs dissipation term that zeros out the appropriate entries. Later we did the same for the ShallowWaterEquations in 1D and 2D as well as for the two-layer shallow water equations.

@ranocha ranocha added the bug Something isn't working label Nov 10, 2023
@ranocha
Copy link
Member

ranocha commented Nov 10, 2023

The "correct" fix for it would be to treat space-dependent terms as first-class citizens, see #358

@andrewwinters5000
Copy link
Member

Definitely, this issue and subsequent workarounds have been around for awhile.

@jlchan
Copy link
Contributor Author

jlchan commented Nov 10, 2023

Great. I'll file a PR to fix it for the quasi-1D SWE and then it should be resolved for that equation too.

@jlchan
Copy link
Contributor Author

jlchan commented Nov 11, 2023

Turns out the LxF dissipation was already implemented correctly for quasi-1D SWE. The issue was with flux for quasi-1D SWE instead (it included a non-conservative term). This will be fixed in #1731.

@jlchan jlchan closed this as completed Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants