Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TVD slope limiters #1662

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

TVD slope limiters #1662

wants to merge 1 commit into from

Conversation

akshaysridhar
Copy link
Member

@akshaysridhar akshaysridhar commented Apr 5, 2024 •
edited
Loading

Update flux correction stencils
Update column and deformation flow examples to use TVD limiters.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@akshaysridhar akshaysridhar requested a review from OsKnoth April 5, 2024 20:54
@akshaysridhar akshaysridhar force-pushed the as/limiter-template branch 2 times, most recently from 0b2a5ce to 656b9a0 Compare April 5, 2024 22:26
@akshaysridhar akshaysridhar changed the title TVD Limiters TVD slope limiters Apr 8, 2024
@akshaysridhar akshaysridhar force-pushed the as/limiter-template branch 2 times, most recently from b9b86e1 to 165c168 Compare November 13, 2024 21:02
@akshaysridhar akshaysridhar marked this pull request as ready for review November 13, 2024 21:02
@akshaysridhar akshaysridhar force-pushed the as/limiter-template branch 2 times, most recently from 9231c2d to 86ed761 Compare November 13, 2024 21:43
@akshaysridhar akshaysridhar force-pushed the as/limiter-template branch 2 times, most recently from dc12510 to dd2a6be Compare November 15, 2024 17:31
charleskawczynski
charleskawczynski reviewed Nov 19, 2024
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these limiters don't seem to prevent negative values:

Screenshot 2024-11-19 at 3 50 48 PM

Is this behavior expected? Should we add more granular unit tests?

The RZeroLimiter seems to work well at removing negative values:

Screenshot 2024-11-19 at 3 51 45 PM

@akshaysridhar
Copy link
Member Author

akshaysridhar commented Nov 19, 2024 •
edited
Loading

@charleskawczynski These are from a class of slope limiting fluxes which combine low-order (monotone) and high-order (not necessarily monotone) fluxes : the RZero here then corresponds to the low order (first order upwind, likely too dissipative to be useful in our simulations) option, and the RMax returns the third order upwind method (which is the current choice of the high-order flux in this method. RHalf equally weights the low / high order methods). (Zalesak, similarly, is a combination of such low / high order fluxes , with flux corrections through an "anti-diffusive" term). This PR introduces the alternative to flux-corrected transport through total-variation diminishing limiters.

@charleskawczynski
Copy link
Member

@charleskawczynski These are from a class of slope limiting fluxes which combine low-order (monotone) and high-order (not necessarily monotone) fluxes : the RZero here then corresponds to the low order (first order upwind, likely too dissipative to be useful in our simulations) option, and the RMax returns the third order upwind method (which is the current choice of the high-order flux in this method. RHalf equally weights the low / high order methods). (Zalesak, similarly, is a combination of such low / high order fluxes , with flux corrections through an "anti-diffusive" term). This PR introduces the alternative to flux-corrected transport through total-variation diminishing limiters.

Ok, could we maybe also plot what the solution looks like without a limiter? That way we can see how each one improves over the baseline.

@charleskawczynski
Copy link
Member

Ok, could we maybe also plot what the solution looks like without a limiter? That way we can see how each one improves over the baseline.

Actually, maybe that is unstable? Perhaps this is fine? I just want to make sure that this is working, and I'd like to have some sort of test to confirm it. The plots look suspiciously ineffective.

@szy21
Copy link
Member

szy21 commented Dec 16, 2024

I'm not the best person to review this. @charleskawczynski @dennisYatunin Could any of you take a look at this PR ASAP? We hope to merge this before the end of tomorrow, thanks!

@akshaysridhar akshaysridhar force-pushed the as/limiter-template branch 7 times, most recently from 9c592f2 to b202585 Compare December 17, 2024 22:49
dennisYatunin
dennisYatunin reviewed Dec 17, 2024
View reviewed changes
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
src/Operators/finitedifference.jl Show resolved Hide resolved
dennisYatunin
dennisYatunin reviewed Dec 17, 2024
View reviewed changes
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
src/Operators/finitedifference.jl Show resolved Hide resolved
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
dennisYatunin
dennisYatunin reviewed Dec 18, 2024
View reviewed changes
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
src/Operators/finitedifference.jl Outdated Show resolved Hide resolved
src/Operators/finitedifference.jl Show resolved Hide resolved
examples/column/vanleer_advection.jl Outdated
Comment on lines 116 to 131
if limiter_method != Operators.AlgebraicMean()
@info "Extrema with $(limiter_method): $(extrema(q_final))"
@assert maximum(q_final) <= FT(1)
@assert isapprox(maximum(q_final .- 1), FT(0), atol = FT(0.05))
@assert isapprox(minimum(q_final .- 0), FT(0), atol = FT(0.05))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I'd expect a lower error than 5%, but maybe if Δt is big for this particular problem, and if there are several stages that each introduce a bit of error, then this result makes sense. Could you also add a similar monotonicity check for the RZeroLimiter in the TVD advection test, and any other limiters that should preserve monotonicity?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this error decrease if you lower the value of Δt? It would be good to have a unit test that demonstrates monotonicity preservation up to machine epsilon precision, if that's possible for a sufficiently small CFL number in this test case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisYatunin Updated with reduced cfl and info statements.

@charleskawczynski
Copy link
Member

Hi @akshaysridhar, just a heads up: I'm going to rebase and push some changes

@akshaysridhar
Copy link
Member Author

akshaysridhar commented Dec 19, 2024 •
edited
Loading

Hi @akshaysridhar, just a heads up: I'm going to rebase and push some changes

Noted; I've added some commits addressing the unit test comments (I'll wait for the rebase update to push). Thanks @charleskawczynski

@akshaysridhar
Copy link
Member Author

Hi @akshaysridhar, just a heads up: I'm going to rebase and push some changes

Noted; I've added some commits addressing the unit test comments (I'll wait for the rebase update to push). Thanks @charleskawczynski

I've pushed these changes @charleskawczynski

@akshaysridhar
Copy link
Member Author

https://buildkite.com/clima/climacore-ci/builds/4847#0193e491-227b-43eb-a2a2-7fbb72315844 @charleskawczynski This job passes locally (runs all deformation flow configs with plots generated at the end of the simulation) but fails on buildkite - the assertion error is part of this pr, the convergence warnings are not (those are from the horizontal limiter implementations and are consistent with main branch outcomes)

@akshaysridhar @charleskawczynski
Introduce abstractions for TVD slope limiter functions (Durran 1999) and
084f693 
van Leer limiters as in Lin(1994)
Update numerical flux stencils to use tvd limiters
Update column examples and references
Update deformation flow example to use limiters

Co-authored-by: Charles Kawczynski <[email protected]>

	modified:   src/Operators/finitedifference.jl

Standard symbols : \scru_space -> u_space

Move docstring

	modified:   examples/column/vanleer_advection.jl
	modified:   src/Operators/finitedifference.jl

	modified:   examples/column/tvd_advection.jl
	modified:   examples/hybrid/sphere/deformation_flow.jl
	modified:   src/Operators/finitedifference.jl

verbose op+method names

	modified:   examples/column/vanleer_advection.jl

Reduce cfl and show eps convergence for mono4, mono5

Remove some eltype conversions

Fix name

Docs formatting

Apply julia formatter

Update domain extent

Fix names, move constraint outside of BCs

Added and updated docs

More fixes + info statements

method -> constraint ; new kwarg

Try Float64

Print info in failing tracer test

Update factor in deformation flow tracer condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennisYatunin dennisYatunin dennisYatunin left review comments

@charleskawczynski charleskawczynski charleskawczynski left review comments

@OsKnoth OsKnoth Awaiting requested review from OsKnoth

@sriharshakandala sriharshakandala Awaiting requested review from sriharshakandala

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
O1.2.4 Finalize numerics for moist si...
Development

Successfully merging this pull request may close these issues.
4 participants
@akshaysridhar @charleskawczynski @szy21 @dennisYatunin