Update elixirs to use default LLF wave speed (PR #2464) #2685
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
| # Up to version 0.13.0, `max_abs_speed_naive` was used as the default wave speed estimate of | ||
| # `const flux_lax_friedrichs = FluxLaxFriedrichs(), i.e., `FluxLaxFriedrichs(max_abs_speed = max_abs_speed_naive)`. | ||
| # `const flux_lax_friedrichs = FluxLaxFriedrichs(), i.e., `FluxLaxFriedrichs()`. | ||
| # In the `StepsizeCallback`, though, the less diffusive `max_abs_speeds` is employed which is consistent with `max_abs_speed`. | ||
| # Thus, we exchanged in PR#2458 the default wave speed used in the LLF flux to `max_abs_speed`. | ||
| # To ensure that every example still runs we specify explicitly `FluxLaxFriedrichs(max_abs_speed_naive)`. | ||
| # To ensure that every example still runs we specify explicitly `FluxLaxFriedrichs()`. | ||
| # We remark, however, that the now default `max_abs_speed` is in general recommended due to compliance with the | ||
| # `StepsizeCallback` (CFL-Condition) and less diffusion. | ||
| surface_flux = FluxLaxFriedrichs(max_abs_speed_naive) | ||
| surface_flux = FluxLaxFriedrichs() |
There was a problem hiding this comment.
Thanks for taking initiative here! The goal would be to delete this comment, though, and just use flux_lax_friedrichs. The issue is that you will most llikely run into a number of CI failures due to slightly different errors.
There was a problem hiding this comment.
So this comment applies to every elixir, i.e., it would be nice to replace in every elixir this comment and switch to flux_lax_friedrichs.
…ositivity elixirs
|
I’ve implemented the requested changes: 1)replaced explicit FluxLaxFriedrichs(...) usage with flux_lax_friedrichs in the elixirs 2)removed the outdated LLF comment where applicable |
| # Up to version 0.13.0, `max_abs_speed_naive` was used as the default wave speed estimate of | ||
| # `const flux_lax_friedrichs = FluxLaxFriedrichs(), i.e., `FluxLaxFriedrichs(max_abs_speed = max_abs_speed_naive)`. | ||
| # In the `StepsizeCallback`, though, the less diffusive `max_abs_speeds` is employed which is consistent with `max_abs_speed`. | ||
| # Thus, we exchanged in PR#2458 the default wave speed used in the LLF flux to `max_abs_speed`. | ||
| # To ensure that every example still runs we specify explicitly `FluxLaxFriedrichs(max_abs_speed_naive)`. | ||
| # We remark, however, that the now default `max_abs_speed` is in general recommended due to compliance with the | ||
| # `StepsizeCallback` (CFL-Condition) and less diffusion. |
There was a problem hiding this comment.
This is what we would like to have for every elixir: Remove the entire comment once you switch fluxes
|
Nice, can you now also format the examples via
|
3 participants
This PR updates elixirs in the CI groups from #2464 to use the new default Local Lax–Friedrichs wave speed by removing explicit max_abs_speed_naive overrides for FluxLaxFriedrichs/Rusanov.
I ran representative elixirs (1D/2D/3D tree, structured, dgmulti, etc.) locally and they all finished with retcode: Success.
On my local macOS + Julia 1.12 setup, test/test_threaded.jl reports 8 failing tests in elixir_euler_positivity.jl (4 passed, 8 failed, 12 total). These failures also appear when I restore that elixir to match origin/main, so they seem environment-specific rather than caused by this PR. CI should clarify the correct behavior.