Skip to content
This repository has been archived by the owner on Mar 1, 2023. It is now read-only.

Yt/boyd vandeven #1359

Merged
merged 1 commit into from
Jul 21, 2020
Merged

Yt/boyd vandeven #1359

merged 1 commit into from
Jul 21, 2020

Conversation

slifer50
Copy link
Contributor

Description

Addition of the Boyd-Vandeven filter.

Me and @thomasgibson have worked on adding the Boyd-Vandeven filter. This currently appears to be working and is only pending the addition of a unit test and some cleanup.

For review by CLIMA developers

  • There are no open pull requests for this already
  • CLIMA developers with relevant expertise have been assigned to review this submission
  • The code conforms to the style guidelines and has consistent naming conventions. julia .dev/format.jl has been run in a separate commit.
  • This code does what it is technically intended to do (all numerics make sense physically and/or computationally)

@bischtob bischtob linked an issue Jul 17, 2020 that may be closed by this pull request
Add Boyd-Vandeven filter to dycore #1350
Closed
3 tasks
@tapios
Copy link
Contributor

tapios commented Jul 17, 2020

Thanks for putting this together, @slifer50!

@tapios
Copy link
Contributor

tapios commented Jul 17, 2020

Looks good to me!

@slifer50
Copy link
Contributor Author

I've done a few tests with the filter to see how it affects the solution of an inviscid rising bubble. The first image shows solution when using the Boyd-Vandeven filter. The second and third images show the solution of the unfiltered flow. The second image showcases the large extrema while the third uses an adjusted scale for a clearer image.
Both of these tests show the presence of element imprinting, with and without the filter.

Screen Shot 2020-07-17 at 10 39 50 PM

Screen Shot 2020-07-17 at 10 57 35 PM

Screen Shot 2020-07-17 at 11 29 08 PM

@slifer50
Copy link
Contributor Author

To investigate the source of the element imprinting I ran the unfiltered rising bubble with Constant diffusions of 75,10, 0.5 and 0. The images are presented in that order.
Screen Shot 2020-07-18 at 12 14 43 AM
Screen Shot 2020-07-18 at 12 29 12 AM
Screen Shot 2020-07-18 at 12 41 44 AM
Screen Shot 2020-07-17 at 11 29 08 PM
We clearly see that the element imprinting seems to be tied to a lack of diffusion in the flow.
This is what I think is happening:
Without enough diffusion in the flow, the effects of the numerical fluxes between elements becomes much more apparent. This is especially since the Rusanov flux is diffusive which might be creating surfaces of diffusion specifically around element boundaries which results in the imprinting we see (note that this does not break the continuity of the solution). This imprinting as shown by the bubble case is also only visible where we have gradients and thus active numerical fluxes. The areas with constant delta_theta do not show any imprinting

@tapios
Copy link
Contributor

tapios commented Jul 19, 2020

To investigate the source of the element imprinting I ran the unfiltered rising bubble with Constant diffusions of 75,10, 0.5 and 0. The images are presented in that order.
Screen Shot 2020-07-18 at 12 14 43 AM
Screen Shot 2020-07-18 at 12 29 12 AM
Screen Shot 2020-07-18 at 12 41 44 AM
Screen Shot 2020-07-17 at 11 29 08 PM
We clearly see that the element imprinting seems to be tied to a lack of diffusion in the flow.
This is what I think is happening:
Without enough diffusion in the flow, the effects of the numerical fluxes between elements becomes much more apparent. This is especially since the Rusanov flux is diffusive which might be creating surfaces of diffusion specifically around element boundaries which results in the imprinting we see (note that this does not break the continuity of the solution). This imprinting as shown by the bubble case is also only visible where we have gradients and thus active numerical fluxes. The areas with constant delta_theta do not show any imprinting

I expect this to improve with numerical fluxes that have proper limits for low-Mach number flow, e.g., Roe fluxes with a Mach number corrections that reduces them to standard upwinding in the low-Mach number limit. Rusanov with the low-Mach number flows such as you have here is extremely over-upwinding.

@smarras79
Copy link
Contributor

Good job @slifer50, thanks for adding this so quickly and having it to work. The filter seems to be working correctly when it comes to stabilizing the solution. I agree with @tapios that the imprinting is related to the flux and the filter could not handle it unless somewhat tuned for that (not worth a try).
After this PR has been approved and is ready to be merged, please, in this order, do the following:

  • run the julia formatter on the files that you modified
  • rebase (I can help you with the process if you don't know the steps)
  • push -f

thanks!

@thomasgibson thomasgibson force-pushed the yt/Boyd-Vandeven branch 2 times, most recently from 61bf0d3 to 8f2166d Compare July 21, 2020 03:42
@bischtob
Copy link
Contributor

@slifer50, can we squash and get this in :-). Looks like it can be part of the codebase now.

@slifer50 @thomasgibson
Add the Boyd-Vandeven filter
02278e4 
co-authored-by: thomasgibson <gibsonthomas1120@hotmail.com>
@bischtob bischtob self-requested a review July 21, 2020 18:32
bischtob
bischtob reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@bischtob bischtob left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomasgibson
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Jul 21, 2020
@bors @charleskawczynski
@blallen @slifer50
Merge #1280 #1321 #1359
4bc03e6 
1280: Add types and unify functions in BalanceLaws r=charleskawczynski a=charleskawczynski

# Description

 - Adds types to balance law (I have no strong opinions about the names here)

```julia
abstract type AbstractStateType end
struct Prognostic <: AbstractStateType end
struct Auxiliary <: AbstractStateType end
struct Gradient <: AbstractStateType end
struct GradientFlux <: AbstractStateType end
struct GradientLaplacian <: AbstractStateType end
struct Hyperdiffusive <: AbstractStateType end
struct UpwardIntegrals <: AbstractStateType end
struct DownwardIntegrals <: AbstractStateType end
```

 - Unifies `vars_state` (BalanceLaws) into a single function
 - Unifies `number_states` (BalanceLaws) into a single function
 - Unifies `create_state` (DGMethods) into a single function
 - Unifies `extract_state` (Diagnostics) into a single function



1321: More pedagogical computation of continuity equation r=blallen a=blallen

# Description

@jm-c pointed out that our current method for computing the continuity equation `∇u = 0` is a little counterintuitive. Here is one possible change we can make to improve the clarity while maintaining computational efficiency at the cost of some extra memory usage.



1359: Yt/boyd vandeven r=thomasgibson a=slifer50

# Description

Addition of the Boyd-Vandeven filter.



Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
Co-authored-by: Brandon Allen <ballen@mit.edu>
Co-authored-by: yassine <yassinetissaoui@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 21, 2020

Build failed (retrying...):

@bors
Copy link
Contributor

bors bot commented Jul 21, 2020

Build succeeded:

@bors bors bot merged commit 4f09146 into master Jul 21, 2020
@bors bors bot deleted the yt/Boyd-Vandeven branch July 21, 2020 21:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tapios tapios tapios left review comments

@bischtob bischtob bischtob left review comments

@fxgiraldo fxgiraldo Awaiting requested review from fxgiraldo

@smarras79 smarras79 Awaiting requested review from smarras79

Assignees

@thomasgibson thomasgibson

@slifer50 slifer50

Labels
AtmosDycore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Boyd-Vandeven filter to dycore
5 participants
@slifer50 @tapios @smarras79 @bischtob @thomasgibson