-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add gradient variable type parameter to AbstractEquationsParabolic
#1409
Changes from all commits
6728af9
1972b6c
2631c96
3be5b06
f358516
c68e0bf
8cbddaf
8b0850f
9b83ead
ea7c4dc
18ec019
254470c
40ae302
c3ffffe
d9381f9
3f76c75
f9930de
8bf5937
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,7 +86,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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if I understand correctly, this modification sets For completeness, would it be necessary to add an appropriate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, for Navier-Stokes the constructor defaults to I don't think we plan on adding conservative gradient variables, so I'd lean towards not implementing |
||
# TODO: parabolic | ||
# 1) For now save gamma and inv(gamma-1) again, but could potentially reuse them from the Euler equations | ||
# 2) Add NGRADS as a type parameter here and in AbstractEquationsParabolic, add `ngradients(...)` accessor function | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a way to make this choice automatic? Otherwise, it will be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.