Port shock capturing with non-conservative terms into Taal

[andrewwinters5000]

This takes the implementation of @amrueda and puts it into Taal

[codecov]

Codecov Report

Merging #256 into dev will increase coverage by 0.24%.
The diff coverage is 96.38%.

@@            Coverage Diff             @@
##              dev     #256      +/-   ##
==========================================
+ Coverage   89.41%   89.66%   +0.24%     
==========================================
  Files          60       60              
  Lines       10585    10702     +117     
==========================================
+ Hits         9465     9596     +131     
+ Misses       1120     1106      -14     

Impacted Files	Coverage Δ
src/equations/2d/ideal_glm_mhd.jl	92.62% <95.65%> (+3.61%)	⬆️
src/solvers/dg/dg_2d.jl	91.71% <95.83%> (+0.74%)	⬆️
src/solvers/dg/2d/dg.jl	93.40% <97.18%> (+0.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 222241f...25367bb. Read the comment docs.

[ranocha]

Looks good in general 👍 Once everything works, please remove old code that you commented out.

[andrewwinters5000]

Looks good in general 👍 Once everything works, please remove old code that you commented out.

Sorry about that, I forgot to be more careful and remove my "reference" routines

[ranocha]

Sorry about that, I forgot to be more careful and remove my "reference" routines

No worries - that's why we have code review 😉

[ranocha]

Looks good to me once my comments below are resolved 👍 Thanks, @amrueda and @andrewwinters5000!

[sloede]

Overall great work! I left a few minor comments or suggestions mostly pertaining to consistency.

One change that affects also current setups is the fact that the SC routines now use non-conservative flux storage even when not using MHD. Have you done a performance check for some Euler tests with Taal before/after to see if and how compute time/memory usage is affected?

[andrewwinters5000]

One change that affects also current setups is the fact that the SC routines now use non-conservative flux storage even when not using MHD. Have you done a performance check for some Euler tests with Taal before/after to see if and how compute time/memory usage is affected?

I have not done any benchmarking in this respect and, to be honest, am not sure how to do this

[ranocha]

I have not done any benchmarking in this respect and, to be honest, am not sure how to do this

I'll do that now and document it for you here.

[amrueda]

I'll do that now and document it for you here.

Thanks, @ranocha. If the current implementation turns to be too inefficient, I can try to improve it in a moment.

[sloede]

If the current implementation turns to be too inefficient, I can try to improve it in a moment.

I don't suspect it will. But Julia has consistently amazed me in the past about how wrong I can be about performance assumptions...

[ranocha]

1. git checkout e7ebf3846b3fd62ee1d0042e130afb50d7fe8e48 (new version)
2. Start julia --threads=1 --check-bounds=no.
3. Execute the following code in the REPL to benchmark the rhs! call at the final state.

   julia> using BenchmarkTools, Revise; using Trixi
   
   julia> trixi_include("examples/2d/elixir_euler_sedov_blast_wave_shockcapturing_amr.jl")
   
   julia> du_test = copy(sol.u[end]); u_test = copy(sol.u[end]);
   
   julia> @benchmark Trixi.rhs!(
             $(du_test),
             $(u_test),
             $(semi),
             $(sol.t[end]))
   BenchmarkTools.Trial:
    memory estimate:  10.48 KiB
    allocs estimate:  67
    --------------
    minimum time:     4.510 ms (0.00% GC)
    median time:      4.646 ms (0.00% GC)
    mean time:        4.699 ms (0.00% GC)
    maximum time:     7.183 ms (0.00% GC)
    --------------
    samples:          1065
    evals/sample:     1
   
   shell> git checkout 222241ff54f8a4ca9876cc1fc25ae262416a4ea0
   
   julia> trixi_include("examples/2d/elixir_euler_sedov_blast_wave_shockcapturing_amr.jl")
   
   julia> @benchmark Trixi.rhs!(
             $(du_test),
             $(u_test),
             $(semi),
             $(sol.t[end]))
   BenchmarkTools.Trial:
    memory estimate:  10.36 KiB
    allocs estimate:  67
    --------------
    minimum time:     4.500 ms (0.00% GC)
    median time:      4.635 ms (0.00% GC)
    mean time:        4.676 ms (0.00% GC)
    maximum time:     5.880 ms (0.00% GC)
    --------------
    samples:          1070
    evals/sample:     1

   Run the @benchmark ... commands multiple times to see whether there are any significant fluctuations.

Follow these steps for both commits you want to compare. The relevant benchmark results I'm mostly looking at
are the median and mean values of the runtime and the memory/allocs estimate. On my system, the differences
of the runtimes are of the order of the fluctuations I get when running the benchmarks multiple times. Since
the memory/allocs are (roughly) the same, I think there is no significant performance regression here.

You can also make it more detailed by benchmarking only the calculation of the volume terms but I think that's
not necessary here.