add gradient variable type parameter to `AbstractEquationsParabolic` #1409

jlchan · 2023-04-25T18:57:44Z

This PR modifies AbstractEquationsParabolic{NDIMS, NVARS} to AbstractEquationsParabolic{NDIMS, NVARS, GradientVariable} so that one can dispatch on (for example) an abstract parabolic equation which uses entropy variable gradients.

Another way to do this would be to introduce a trait system, similar to has_nonconservative_terms, but I think that is more complex than it needs to be.

ranocha

Thanks! Which use case do you have in mind that requires this change?

ranocha · 2023-04-26T04:21:05Z

docs/literate/src/files/adding_new_parabolic_terms.jl

+# the gradient with respect to a different set of variables; see, for example, the implementation of
+# [`CompressibleNavierStokesDiffusion2D`](@ref), which can utilize either "primitive" or "entropy" variables.
+
+struct ConstantAnisotropicDiffusion2D{E, T} <: Trixi.AbstractEquationsParabolic{2, 1, GradientVariablesConservative}


Can we find a way to make this choice automatic? Otherwise, it will be a breaking change.

As far as I know, no, since I think it would require the ability to set "default" parameters. https://discourse.julialang.org/t/type-parameters-by-default/45577

I can hold off on this PR until we are ready for a breaking release. The stuff that would require this is an optimization, and doesn't prevent me from implementing other features.

src/equations/equations.jl

jlchan · 2023-04-26T04:31:12Z

The use case would be for flux differencing schemes, where the projected entropy variables are computed as part of the inviscid scheme. Adding a GradientVariables parameter would enable dispatch on GradientVariablesEntropy for DGMultiFluxDiff schemes to avoid recomputing the entropy variable projection a second time.

src/equations/equations_parabolic.jl

sloede · 2023-04-26T04:38:59Z

The use case would be for flux differencing schemes, where the projected entropy variables are computed as part of the inviscid scheme. Adding a GradientVariables parameter would enable dispatch on GradientVariablesEntropy for DGMultiFluxDiff schemes to avoid recomputing the entropy variable projection a second time.

We discussed this in yesterday's meeting. We weren't sure whether there would be another option that does not introduce a new type parameter that will allow Jesse to use the same dispatch (e.g., by adding additional arguments to function calls for dispatch, or type traits, or...) but without making the type system more complicated.

We thus figured it would be the easiest if we see a PR and then can possible iterate on it and discuss alternatives.

ranocha · 2023-04-26T12:43:16Z

The use case would be for flux differencing schemes, where the projected entropy variables are computed as part of the inviscid scheme. Adding a GradientVariables parameter would enable dispatch on GradientVariablesEntropy for DGMultiFluxDiff schemes to avoid recomputing the entropy variable projection a second time.

We discussed this in yesterday's meeting. We weren't sure whether there would be another option that does not introduce a new type parameter that will allow Jesse to use the same dispatch (e.g., by adding additional arguments to function calls for dispatch, or type traits, or...) but without making the type system more complicated.

We thus figured it would be the easiest if we see a PR and then can possible iterate on it and discuss alternatives.

I see, thanks! I'm fine with this but we may wait a little bit to see whether there are other breaking changes we would like to introduce in the near future.

sloede · 2023-04-26T13:06:36Z

The use case would be for flux differencing schemes, where the projected entropy variables are computed as part of the inviscid scheme. Adding a GradientVariables parameter would enable dispatch on GradientVariablesEntropy for DGMultiFluxDiff schemes to avoid recomputing the entropy variable projection a second time.

We discussed this in yesterday's meeting. We weren't sure whether there would be another option that does not introduce a new type parameter that will allow Jesse to use the same dispatch (e.g., by adding additional arguments to function calls for dispatch, or type traits, or...) but without making the type system more complicated.
We thus figured it would be the easiest if we see a PR and then can possible iterate on it and discuss alternatives.

I see, thanks! I'm fine with this but we may wait a little bit to see whether there are other breaking changes we would like to introduce in the near future.

Just so I understand it right: It is a breaking change, because we add a type parameter to the AbstractEquationsParabolic type and there's no way in Julia to set a default such that existing user code would continue to work without modifications?

jlchan · 2023-04-26T13:44:04Z

That's correct. We don't technically export, AbstractParabolicEquations, but it does show up in the literate docs, so it is in some sense "public".

ranocha · 2023-04-26T14:07:29Z

Yes, that's my point of view. We describe it in the docs, so it's stable API. This PR breaks the way how we tell people to set up their own parabolic systems, so we need to make a breaking release.

codecov · 2023-05-02T15:23:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c79fa43) 72.65% compared to head (f9930de) 80.79%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
+ Coverage   72.65%   80.79%   +8.14%     
==========================================
  Files         431      431              
  Lines       34637    34645       +8     
==========================================
+ Hits        25164    27991    +2827     
+ Misses       9473     6654    -2819

Flag	Coverage Δ
unittests	`80.79% <ø> (+8.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/Trixi.jl	`43.48% <ø> (ø)`
src/equations/compressible_navier_stokes_1d.jl	`0.00% <ø> (ø)`
src/equations/compressible_navier_stokes_2d.jl	`41.67% <ø> (ø)`
src/equations/compressible_navier_stokes_3d.jl	`0.00% <ø> (ø)`
src/equations/equations.jl	`98.00% <ø> (+8.00%)`	⬆️
src/equations/equations_parabolic.jl	`100.00% <ø> (ø)`

... and 70 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewwinters5000

I just left two questions (in a single comment) for clarification. If this is to be a breaking change it is not time pertinent (I think).

andrewwinters5000 · 2023-08-01T11:12:05Z

src/equations/compressible_navier_stokes_3d.jl

@@ -78,7 +78,7 @@ w_2 = \frac{\rho v_1}{p},\, w_3 = \frac{\rho v_2}{p},\, w_4 = \frac{\rho v_3}{p}
 """
 struct CompressibleNavierStokesDiffusion3D{GradientVariables, RealT <: Real,
                                           E <: AbstractCompressibleEulerEquations{3}} <:
-       AbstractCompressibleNavierStokesDiffusion{3, 5}
+       AbstractCompressibleNavierStokesDiffusion{3, 5, GradientVariables}


So if I understand correctly, this modification sets GradientVariablesConservative to be the default within equations_parabolic.jl only for the Laplace diffusion whereas for Navier-Stokes we still need to pass whichever gradient_variables we want into the constructor?

For completeness, would it be necessary to add an appropriate convert_derivative_to_primitive function for the GradientVariablesConservative in the Navier-Stokes file?

No, for Navier-Stokes the constructor defaults to GradientVariablesPrimitive.

https://github.com/jlchan/Trixi.jl/blob/c3ffffe0090a0913edc6e7829686b7a77a8d5bc0/src/equations/compressible_navier_stokes_2d.jl#L104-L106

I don't think we plan on adding conservative gradient variables, so I'd lean towards not implementing convert_derivative_to_primitive for conservative gradient variables.

…_parabolic_type_parameter

ranocha

Thanks! This should be ready to go, isn't it, @jlchan ?

ranocha

Wait, sorry, I forgot something - could you please add a NEWS.md entry?
I also changed the base of the PR to the v0.6 development branch @sloede created.

jlchan · 2023-11-07T14:08:30Z

Thanks! And yes, I'll add this to NEWS.md

jlchan · 2023-11-08T15:17:26Z

Merging as per comments on Slack.

…1409) * add gradient variable type parameter * fix parabolic literate test * remove trailing comment * remove unnecessary abstract type * move gradient variable structs * formatting * fix dropped changes * try to fix doc tests * fixing navier stokes 1D * formatting * remove duplicate GradientVariablesPrimitive/Entropy definition * update news

* Breaking changes HLL MHD * format * format examples * hlle * fix * news, tests, example changes * fmt * remove left-right-biased flux from test * Set version to v0.6.0 * Migrate neural network-based indicators to new repository (#1701) * Remove all neural network indicator stuff from `src/` * Migrate neural network tests * Migrate neural network examples * Migrate test dependencies * Update NEWS.md * Fix typo * Remove Requires.jl-based use of Flux.jl * Fix formatting * Add migration of indicators to section with breaking changes --------- Co-authored-by: Hendrik Ranocha <[email protected]> * fix hlle noncartesian 2d * remove parantheses * correct test vals * Make parabolic terms nonexperimental (#1714) * Make parabolic terms non-experimental * Make NSE a separate item * Add MPI to supported features * Mention that parabolic terms are now officially supported in NEWS.md Co-authored-by: Hendrik Ranocha <[email protected]> * Deprecate some `DGMultiMesh` constructors (#1709) * remove previously deprecated functions * fix typo in NEWS.md about deprecation vs removal * fix literate tutorial * removing other deprecation * format * Revert "fix typo in NEWS.md about deprecation vs removal" This reverts commit 6b03020. * add gradient variable type parameter to `AbstractEquationsParabolic` (#1409) * add gradient variable type parameter * fix parabolic literate test * remove trailing comment * remove unnecessary abstract type * move gradient variable structs * formatting * fix dropped changes * try to fix doc tests * fixing navier stokes 1D * formatting * remove duplicate GradientVariablesPrimitive/Entropy definition * update news * bring downloads back * fix failing test * fmt --------- Co-authored-by: Michael Schlottke-Lakemper <[email protected]> Co-authored-by: Hendrik Ranocha <[email protected]>

…1409) * add gradient variable type parameter * fix parabolic literate test * remove trailing comment * remove unnecessary abstract type * move gradient variable structs * formatting * fix dropped changes * try to fix doc tests * fixing navier stokes 1D * formatting * remove duplicate GradientVariablesPrimitive/Entropy definition * update news

* Breaking changes HLL MHD * format * format examples * hlle * fix * news, tests, example changes * fmt * remove left-right-biased flux from test * Set version to v0.6.0 * Migrate neural network-based indicators to new repository (#1701) * Remove all neural network indicator stuff from `src/` * Migrate neural network tests * Migrate neural network examples * Migrate test dependencies * Update NEWS.md * Fix typo * Remove Requires.jl-based use of Flux.jl * Fix formatting * Add migration of indicators to section with breaking changes --------- Co-authored-by: Hendrik Ranocha <[email protected]> * fix hlle noncartesian 2d * remove parantheses * correct test vals * Make parabolic terms nonexperimental (#1714) * Make parabolic terms non-experimental * Make NSE a separate item * Add MPI to supported features * Mention that parabolic terms are now officially supported in NEWS.md Co-authored-by: Hendrik Ranocha <[email protected]> * Deprecate some `DGMultiMesh` constructors (#1709) * remove previously deprecated functions * fix typo in NEWS.md about deprecation vs removal * fix literate tutorial * removing other deprecation * format * Revert "fix typo in NEWS.md about deprecation vs removal" This reverts commit 6b03020. * add gradient variable type parameter to `AbstractEquationsParabolic` (#1409) * add gradient variable type parameter * fix parabolic literate test * remove trailing comment * remove unnecessary abstract type * move gradient variable structs * formatting * fix dropped changes * try to fix doc tests * fixing navier stokes 1D * formatting * remove duplicate GradientVariablesPrimitive/Entropy definition * update news * bring downloads back * fix failing test * fmt --------- Co-authored-by: Michael Schlottke-Lakemper <[email protected]> Co-authored-by: Hendrik Ranocha <[email protected]>

add gradient variable type parameter

6728af9

jlchan marked this pull request as draft April 25, 2023 18:57

fix parabolic literate test

1972b6c

jlchan marked this pull request as ready for review April 26, 2023 02:22

jlchan requested review from andrewwinters5000, sloede and ranocha April 26, 2023 02:22

ranocha requested changes Apr 26, 2023

View reviewed changes

ranocha added the breaking label Apr 26, 2023

sloede reviewed Apr 26, 2023

View reviewed changes

src/equations/equations_parabolic.jl Outdated Show resolved Hide resolved

src/equations/equations_parabolic.jl Outdated Show resolved Hide resolved

jlchan added 2 commits April 26, 2023 08:56

remove trailing comment

2631c96

remove unnecessary abstract type

3be5b06

jlchan and others added 2 commits April 26, 2023 09:57

move gradient variable structs

f358516

Merge branch 'main' into jc/gradient_variables_parabolic_type_parameter

c68e0bf

This was referenced Apr 26, 2023

DGMulti support for curved meshes and parabolic terms #1400

Merged

Release v0.6 #1310

Closed

Merge branch 'main' into jc/gradient_variables_parabolic_type_parameter

8cbddaf

jlchan and others added 6 commits June 16, 2023 11:24

Merge branch 'main' into jc/gradient_variables_parabolic_type_parameter

8b0850f

formatting

9b83ead

fix dropped changes

ea7c4dc

Merge branch 'main' into jc/gradient_variables_parabolic_type_parameter

18ec019

try to fix doc tests

254470c

Merge branch 'main' into jc/gradient_variables_parabolic_type_parameter

40ae302

andrewwinters5000 reviewed Aug 1, 2023

View reviewed changes

jlchan added 4 commits November 6, 2023 16:27

Merge remote-tracking branch 'origin/main' into jc/gradient_variables…

c3ffffe

…_parabolic_type_parameter

fixing navier stokes 1D

d9381f9

formatting

3f76c75

remove duplicate GradientVariablesPrimitive/Entropy definition

f9930de

ranocha approved these changes Nov 7, 2023

View reviewed changes

ranocha changed the base branch from main to v0.6-dev November 7, 2023 06:12

ranocha requested changes Nov 7, 2023

View reviewed changes

update news

8bf5937

jlchan requested a review from ranocha November 7, 2023 14:34

jlchan merged commit 32c787d into trixi-framework:v0.6-dev Nov 8, 2023
26 of 29 checks passed

sloede mentioned this pull request Nov 9, 2023

Staging for breaking release v0.6.0 #1705

Merged

2 tasks

ranocha mentioned this pull request Nov 10, 2023

Release v0.7 #1726

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gradient variable type parameter to `AbstractEquationsParabolic` #1409

add gradient variable type parameter to `AbstractEquationsParabolic` #1409

jlchan commented Apr 25, 2023 •

edited

Loading

ranocha left a comment

ranocha Apr 26, 2023

jlchan Apr 26, 2023

jlchan commented Apr 26, 2023

sloede commented Apr 26, 2023

ranocha commented Apr 26, 2023

sloede commented Apr 26, 2023

jlchan commented Apr 26, 2023

ranocha commented Apr 26, 2023

codecov bot commented May 2, 2023 •

edited

Loading

andrewwinters5000 left a comment

andrewwinters5000 Aug 1, 2023

jlchan Nov 6, 2023 •

edited

Loading

ranocha left a comment

ranocha left a comment

jlchan commented Nov 7, 2023

jlchan commented Nov 8, 2023

add gradient variable type parameter to AbstractEquationsParabolic #1409

add gradient variable type parameter to AbstractEquationsParabolic #1409

Conversation

jlchan commented Apr 25, 2023 • edited Loading

ranocha left a comment

Choose a reason for hiding this comment

ranocha Apr 26, 2023

Choose a reason for hiding this comment

jlchan Apr 26, 2023

Choose a reason for hiding this comment

jlchan commented Apr 26, 2023

sloede commented Apr 26, 2023

ranocha commented Apr 26, 2023

sloede commented Apr 26, 2023

jlchan commented Apr 26, 2023

ranocha commented Apr 26, 2023

codecov bot commented May 2, 2023 • edited Loading

Codecov Report

andrewwinters5000 left a comment

Choose a reason for hiding this comment

andrewwinters5000 Aug 1, 2023

Choose a reason for hiding this comment

jlchan Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

jlchan commented Nov 7, 2023

jlchan commented Nov 8, 2023

add gradient variable type parameter to `AbstractEquationsParabolic` #1409

add gradient variable type parameter to `AbstractEquationsParabolic` #1409

jlchan commented Apr 25, 2023 •

edited

Loading

codecov bot commented May 2, 2023 •

edited

Loading

jlchan Nov 6, 2023 •

edited

Loading