-
Notifications
You must be signed in to change notification settings - Fork 186
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
Maximum-principle-satisfying WENO scheme #2557
Conversation
This is great to see! I imagine we could use this for the height field in |
yeah, I guess in that case you don't want to set an upper bound so we can use |
Just a small plea to make this implementation compatible with / moves us towards the refactor envisioned in #2454 |
return 1/Vᶜᶜᶜ(i, j, k, grid) * (div_x + div_y + div_z) | ||
end | ||
|
||
@inline function bounded_tracer_flux_divergence_x(i, j, k, grid, advection::BoundPreservingScheme, u, c) |
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.
This looks great! It seems like these three funcionts are almost identical except for u -> v -> w. Is that true?
If yes, I wonder if there is away to merge these to one, rather than having three functions that do the same thing?
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.
Indices, areas and interpolations differ. I can try merging it with some metaprogramming
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.
Not necessary. This is maybe cleaner but thought I would make the observation.
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.
I prefer having separate functions over metaprogramming in this case.
@francispoulin the difference between x, y, z is that they depend on indices i, j, k respectively. So we also have advective_tracer_flux_x, etc.
@@ -26,7 +30,7 @@ Weighted Essentially Non-Oscillatory (WENO) fifth-order advection scheme. | |||
|
|||
$(TYPEDFIELDS) | |||
""" | |||
struct WENO5{FT, XT, YT, ZT, XS, YS, ZS, VI, WF} <: AbstractUpwindBiasedAdvectionScheme{2} | |||
struct WENO5{FT, XT, YT, ZT, XS, YS, ZS, VI, WF, PP} <: AbstractUpwindBiasedAdvectionScheme{2} |
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.
Is this the design we will want when we do #2454 ? If we can avoid digging ourselves into a deeper hole we should, but maybe it's not possible without a lot of effort...
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.
I am not still completely sure how #2454 will go along, this change only modifies the div_Uc
so I guess it might be easy to adapt in the future
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.
In #2454 we will disentangle vector invariant from WENO5, so VI
will go away. It might still make sense to blend WENO5
with flux-limited WENO5, however. It's just that if we have nth order WENO, we would also have to figure out how to do nth order flux limited WENO if they are entangled.
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.
I think the limiter will be independent on the order of the original scheme. You always want to blend your scheme into a first order upwind (when crossing bounds)
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.
ok, great!
To support different bounds for different tracers, let's allow the advection scheme to be a |
I think it is already like this for the
(I just found out) |
I guess it is better to change the |
For NonhydrostaticModel you'll have to come up with an API / syntax for momentum versus tracers. Definitely not a bad idea, but we don't want to abuse people with too many syntax changes so we may want to discuss what that would look like. |
Heh, I probably wrote that but then forgot! Good rediscovery. That seems like a decent API --- what do you think? |
I like it! I changed the NonhydrostaticModel to be the same. A way to facilitate users is that we leave the option to specify a global what do you guys think? |
I think we should do that in a different PR because it's a major, major breaking change to the most popular model! As for the syntax I'd prefer --- I would prefer one keyword argument I also feel it's better to just specify |
@@ -50,7 +55,8 @@ end | |||
""" | |||
NonhydrostaticModel(; grid, | |||
clock = Clock{eltype(grid)}(0, 0, 1), | |||
advection = CenteredSecondOrder(), | |||
momentum_advection = CenteredSecondOrder(), | |||
tracer_advection = CenteredSecondOrder(), |
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.
I prefer one kwarg advection
--- and we should do this in a different PR.
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.
Ok, then we can change it in a different PR (for the moment then separate bounds
can be used only in the HydrostaticFreeSurfaceModel
)
src/Advection/positivity_preserving_tracer_advection_operators.jl
Outdated
Show resolved
Hide resolved
src/Advection/positivity_preserving_tracer_advection_operators.jl
Outdated
Show resolved
Hide resolved
p̃ = (cᵢⱼ - ω̂₁ * c₋ᴿ - ω̂ₙ * c₊ᴸ) / (1 - 2ω̂₁) | ||
M = max(p̃, c₊ᴸ, c₋ᴿ) | ||
m = min(p̃, c₊ᴸ, c₋ᴿ) | ||
θ = min(abs((upper_limit - cᵢⱼ)/(M - cᵢⱼ + ε₂)), abs((lower_limit - cᵢⱼ)/(m - cᵢⱼ + ε₂)), 1.0) |
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.
θ = min(abs((upper_limit - cᵢⱼ)/(M - cᵢⱼ + ε₂)), abs((lower_limit - cᵢⱼ)/(m - cᵢⱼ + ε₂)), 1.0) | |
θ = min(abs((upper_limit - cᵢⱼ)/(M - cᵢⱼ + ε₂)), abs((lower_limit - cᵢⱼ)/(m - cᵢⱼ + ε₂)), one(grid)) |
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
Following https://royalsocietypublishing.org/doi/epdf/10.1098/rspa.2011.0153 (section 5)
At the moment this is just a scheme check, so I have implemented it in 1D (and only in the horizontal direction)
For the moment this script
returns
for usual
WENO5
and
for
WENO5(bounds = (0, 1))
So it seems like this scheme, even in its 1D formulation, might help with our tracer bound issues.