Save additional node variables in SaveSolutionCallback
#2298
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2298 +/- ##
=======================================
Coverage 96.91% 96.92%
=======================================
Files 492 492
Lines 40201 40227 +26
=======================================
+ Hits 38960 38986 +26
Misses 1241 1241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Currently, the PR is somewhat inconsistent as for hyperbolic equations there is a dispatch based on the volume integral type, and for the parabolic equations we use an if-clause. This should be unified IMO. |
| if typeof(volume_integral) == VolumeIntegralSubcellLimiting | ||
| # While for the element-wise limiting with `VolumeIntegralShockCapturingHG` the indicator is | ||
| # called here to get up-to-date values for IO, this is not easily possible in this case | ||
| # because the calculation is very integrated into the method. | ||
| # See also https://github.com/trixi-framework/Trixi.jl/pull/1611#discussion_r1334553206. | ||
| # Therefore, the coefficients at `t=t^{n-1}` are saved. Thus, the coefficients of the first | ||
| # stored solution (initial condition) are not yet defined and were manually set to `NaN`. | ||
| get_node_variables!(node_variables, volume_integral.limiter, volume_integral, | ||
| equations) | ||
| else |
There was a problem hiding this comment.
I agree with you that we should use dispatch here instead of the of clause.
There was a problem hiding this comment.
Yeah, or we add an if-clause for the hyperbolic case - might be less code, but also less clean? I am not sure what is the better way here.
There was a problem hiding this comment.
I tend towards using dispatch as then the method can be extended (from outside Trixi.jl) for other types of volume integrals.
|
Maybe it also makes sense to add new functions |
|
@JoshuaLampert could you maybe also take a look and leave your thoughts here? |
| if typeof(volume_integral) == VolumeIntegralSubcellLimiting | ||
| # While for the element-wise limiting with `VolumeIntegralShockCapturingHG` the indicator is | ||
| # called here to get up-to-date values for IO, this is not easily possible in this case | ||
| # because the calculation is very integrated into the method. | ||
| # See also https://github.com/trixi-framework/Trixi.jl/pull/1611#discussion_r1334553206. | ||
| # Therefore, the coefficients at `t=t^{n-1}` are saved. Thus, the coefficients of the first | ||
| # stored solution (initial condition) are not yet defined and were manually set to `NaN`. | ||
| get_node_variables!(node_variables, volume_integral.limiter, volume_integral, | ||
| equations) | ||
| else |
There was a problem hiding this comment.
I tend towards using dispatch as then the method can be extended (from outside Trixi.jl) for other types of volume integrals.
| node_variables = Dict{Symbol, Any}() | ||
| # Add `:vorticity` key to `node_variables` dictionary. | ||
| # The actual values are then computed during the `SaveSolutionCallback`. | ||
| node_variables[:vorticity] = nothing |
There was a problem hiding this comment.
I wonder whether this is the best interface. It feels a bit odd to create a dictionary and set the entry to nothing. One alternative would be to only pass a list of symbols, which can be used internally to create the dict. This is maybe a bit similar to extra_analysis_errors from the AnalysisCallback (In this case, I would probably rename the argument to something like extra_node_variables). Another alternative, which would allow for even more flexibility is to pass a list of functions with a certain signature. These could either we user-defined or pre-defined like a function computing the vorticity. I am not sure, however, if this approach is feasible in general. It would be a bit similar to extra_analysis_integrals from the AnalysisCallback.
There was a problem hiding this comment.
Agreed, the dict should be created internally.
Hm passing in functions would be nice - I think it should be doable.
A more fundamental design choice is probably that we would want another type of solution variables, which truly depend on the solution vector u_ode (the subcell coefficients do not directly). This comes with more changes in the implementation, but is the much cleaner way I think.
First attempt to save additional stuff in the solution callback. Not sure if the current implementation works for all cases already.
This could be extended to other quantities such as local Mach number, temperature, ...