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

More pedagogical computation of continuity equation #1321

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

blallen
Copy link
Contributor

@blallen blallen commented Jul 2, 2020

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.

I have

  • Written and run all necessary tests with CLIMA by including tests/runtests.jl
  • Followed all necessary style guidelines and run julia .dev/climaformat.jl .
  • Updated the documentation to reflect changes from this PR.

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)

@blallen blallen added Ocean 🌊 "How inappropriate to call this planet Earth when it is quite clearly Ocean" Refactor Non-behavior changing labels Jul 2, 2020
@blallen blallen self-assigned this Jul 2, 2020
@blallen
Copy link
Contributor Author

blallen commented Jul 2, 2020

bors try

bors bot added a commit that referenced this pull request Jul 2, 2020
@blallen
Copy link
Contributor Author

blallen commented Jul 2, 2020

@jm-c and @christophernhill, take a look and let me know what you think. This method is kinda half-way between what is in @jm-c branch and in master.

@bors
Copy link
Contributor

bors bot commented Jul 2, 2020

@christophernhill
Copy link
Member

@blallen how does this map to

https://github.com/jm-c/CLIMA/blob/jmc/split_explicit_draft/src/Ocean/SplitExplicit01/Continuity3dModel.jl

should we get in sync using Continuity3dModel.jl before changing to another new way?

@blallen
Copy link
Contributor Author

blallen commented Jul 7, 2020

@blallen how does this map to

https://github.com/jm-c/CLIMA/blob/jmc/split_explicit_draft/src/Ocean/SplitExplicit01/Continuity3dModel.jl

should we get in sync using Continuity3dModel.jl before changing to another new way?

In terms of the math, the only difference is that between calculating a divergence directly and calculating it from the trace of the gradient tensor, e.g. none.

Algorithmically, this uses more memory than JMC's way (altho I just realized I can make a fix to make it use the same or less (will push it soon)) but saves us the step of having to do another iteration across all the elements. Seeing as we currently only do two of those per time step, adding a third is an ~50% increase in compute time.

The main advantage of JMC's method is that we can choose the numerical flux for the continuity equation separately from that of the rest of the gradients calculated. Currently, this just means we can do central or Rusanov fluxes with JMC's method, but only central with mine. In the future, this might be a bigger difference, but I also hope/expect that we will have more flexibility in specifying the numerical fluxes for gradients by the time we have the more complicated numerical fluxes.

@christophernhill
Copy link
Member

christophernhill commented Jul 7, 2020 via email

@blallen
Copy link
Contributor Author

blallen commented Jul 7, 2020

Can we mark it WIP for now? I think we want to be able to check reference numbers for things we merge?

This is marked as a draft PR :) But this change doesn't change any of the state values in the state check. I simply turned off the gradient check for now, as I am adding new gradient variables so the order changes and the state check fails. We can still check reference numbers for PRs on top of this one.

@blallen blallen force-pushed the bla/ocean_continuity branch 5 times, most recently from 2f891a5 to 5ba1ce7 Compare July 21, 2020 18:36
@blallen blallen marked this pull request as ready for review July 21, 2020 18:46
@blallen
Copy link
Contributor Author

blallen commented Jul 21, 2020

bors r+

bors bot added a commit that referenced this pull request Jul 21, 2020
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

@bors bors bot merged commit 4f09146 into CliMA:master Jul 21, 2020
@blallen blallen deleted the bla/ocean_continuity branch July 23, 2020 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ocean 🌊 "How inappropriate to call this planet Earth when it is quite clearly Ocean" Refactor Non-behavior changing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants