Add auxiliary variables #2348
Add auxiliary variables #2348
Conversation
- add container for node and surface auxiliary variables - add initializer to SemidiscretizationHyperbolic - dispatch on have_auxiliary_node_vars for 2D TreeMesh rhs!
to demonstrate how auxiliary variables can be used instead of augmenting the solution state vector
ŕesults have to be identical to the corresponing AcousticPerturbationEquations2D results
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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2348 +/- ##
==========================================
- Coverage 96.93% 96.66% -0.28%
==========================================
Files 500 504 +4
Lines 41499 41942 +443
==========================================
+ Hits 40226 40540 +314
- Misses 1273 1402 +129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
As discussed today, it's great to see this is coming to Trixi.jl! Feel free to ping me in case of questions or for a review! |
|
@knstmrd is this maybe helpful for you? |
|
CI now passes. |
|
Maybe unrelated, but what was the idea behind #576? |
|
As discussed in the Trixi.jl meeting, it would be great if the first round of reviews could be done by @tristanmontoya 😊 |
Yes, I will get to that soon! |
There was a problem hiding this comment.
Nice work! This seems pretty much ready to go as far as I am concerned, aside from some suggestions regarding variable naming and documentation. My suggestion at this point is to leave the names of cache.auxiliary_variables and create_cache_auxiliary as-is, but anywhere we have auxiliary_node_vars or auxiliary_surface_node_vars, that could just be shortened to aux_node_vars or aux_surface_node_vars, since "vars" is already an abbreviation.
I was also wondering, for problems without auxiliary variables, does this add any performance overhead? It doesn't look like it would be significant, but might be good to double check.
Finally, am I correct that the auxiliary variables are recomputed, rather than interpolated, when the mesh is adapted? Does this give a different result than adding them to the solution state u, since in the latter case they would be interpolated alongside the conservative variables?
| @@ -204,7 +206,8 @@ See also https://github.com/trixi-framework/Trixi.jl/issues/1671#issuecomment-17 | |||
| =# | |||
| @inline function weak_form_kernel!(du, u, | |||
| element, mesh::TreeMesh{2}, | |||
| nonconservative_terms::False, equations, | |||
| nonconservative_terms::False, | |||
| auxiliary_node_var::False, equations, | |||
There was a problem hiding this comment.
I would be consistent here with the naming of this parameter in other methods. In most places, you have have_auxiliary_node_vars. If you want to be consistent with nonconservative_terms, then maybe it should just be auxiliary_node_vars, but this could be confused with the actual variables rather than just a boolean (I would actually prefer to change nonconservative_terms to have_nonconservative_terms but that is a separate issue).
| @@ -456,6 +456,44 @@ function get_data_1d(original_nodes, unstructured_data, nvisnodes, reinterpolate | |||
| vcat(original_nodes[1, 1, :], original_nodes[1, end, end]) | |||
| end | |||
|
|
|||
| @inline function apply_solution_variables(u, solution_variables, | |||
There was a problem hiding this comment.
Can you add a comment documenting what this function does?
| n_auxiliary_node_vars(equations) | ||
|
|
||
| Number of auxiliary variables used by `equations`. This function needs to be specialized | ||
| only if equations has auxiliary variables. |
There was a problem hiding this comment.
| only if equations has auxiliary variables. | |
| only if `equations` has auxiliary variables. |
| have_auxiliary_node_vars(equations) | ||
| Trait function determining whether `equations` need to access additional auxiliary | ||
| variables. | ||
| The return value will be `True()` or `False()` to allow dispatching on the return type. |
There was a problem hiding this comment.
There should probably be some documentation of the fact that auxiliary variables currently only work with TreeMesh{2}, which would be updated as we add support for other meshes. I am not sure where that should go - perhaps here?
|
This looks quite comprehensive - we should be really careful if this is breaking or not. |
I think I requested that, but I can't remember why now. |
Add a container holding auxiliary node and surface node variables to semidiscretizations'
cache, s.t. they can be used when computingrhs!.As a first step,
AcousticPerturbationEquations2DAuxVarsare implemented. This is to demonstrate how auxiliary variables can be used instead of augmenting the solution state vector, as previously done, e.g., inAcousticPerturbationEquations2D.This feature should ultimately be usable by
To discuss:
aux_varsvs.auxiliary_variables(and derived names such asget_auxiliary_surface_node_vars)Closes #1623, closes #497, related #358