Add types and unify functions in BalanceLaws #1280

charleskawczynski · 2020-06-23T19:25:36Z

Description

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

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

This PR also does a replace-all for state_conservative -> state_prognostic.

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 · 2020-06-23T19:49:55Z

Maybe rename

struct VerticalIntegrals <: AbstractStateType end
struct ReverseVerticalIntegrals <: AbstractStateType end

to

struct UpwardIntegrals <: AbstractStateType end
struct DownwardIntegrals <: AbstractStateType end

in order to have more descriptive names? You only know which direction an integral is going based on looking at the kernels.

OTOH, we maybe want to rewrite the integral kernels so you just specify the direction for each variable and don't require a corresponding VerticalIntegral variable for each ReverseVerticalIntegral variable.

More comments coming later.

experiments/OceanBoxGCM/homogeneous_box.jl

blallen · 2020-06-23T19:58:40Z

Why do we need to pass FT as an argument to vars_state()? Isn't @vars just a short cut to various views/elements of an array? Why does it need to know the type of the elements in the array?

On a similar note, could we make vars_state a callable object? That way we could just store FT in the struct and not need to pass it to every function call.

charleskawczynski · 2020-06-23T20:05:04Z

Why do we need to pass FT as an argument to vars_state()? Isn't @vars just a short cut to various views/elements of an array? Why does it need to know the type of the elements in the array?

Variable templates needs to know the type (I think it may be needed for MArrays) and rank of each variable.

On a similar note, could we make vars_state a callable object? That way we could just store FT in the struct and not need to pass it to every function call.

🤷‍♂️ I think then we'd have to pass vs = vars_state() through the call graph, which doesn't seem desirable. @simonbyrne ?

charleskawczynski · 2020-06-23T20:05:34Z

struct UpwardIntegrals <: AbstractStateType end, struct DownwardIntegrals <: AbstractStateType end

❤️

charleskawczynski · 2020-06-23T20:09:24Z

OTOH, we maybe want to rewrite the integral kernels so you just specify the direction for each variable and don't require a corresponding VerticalIntegral variable for each ReverseVerticalIntegral variable.

We could make another supertype :)

blallen · 2020-06-23T20:23:11Z

OTOH, we maybe want to rewrite the integral kernels so you just specify the direction for each variable and don't require a corresponding VerticalIntegral variable for each ReverseVerticalIntegral variable.

We could make another supertype :)

Sure, but how kernel_reverse_indefinite_stack_integral! works is that it literally flips the sign on an already calculated integral.

kernel_indefinite_stack_integral! actually does \int_z1^z (integrand) but kernel_indefinite_stack_integral! does \int_z^z2 (integrand) - \int_z1^z2 (integrand)`. After typing that, I realize links to actual code might be better:

https://github.com/CliMA/ClimateMachine.jl/blob/master/src/Numerics/DGMethods/DGModel_kernels.jl#L1723-L1730
versus
https://github.com/CliMA/ClimateMachine.jl/blob/master/src/Numerics/DGMethods/DGModel_kernels.jl#L1808-L1841

They really aren't equivalent operations at this point.

charleskawczynski · 2020-06-23T20:55:20Z

They really aren't equivalent operations at this point.

Ah, I misunderstood your original statement:

maybe want to rewrite the integral kernels so you just specify the direction for each variable and don't require a corresponding VerticalIntegral variable for each ReverseVerticalIntegral variable.

Yes, that seems reasonable. I imagine we could optionally pack this information into VerticalIntegral/ReverseVerticalIntegral, but what your suggesting sounds better I think.

charleskawczynski · 2020-06-24T16:43:15Z

bors try

bors · 2020-06-24T18:28:30Z

try

Build failed:

format

charleskawczynski · 2020-06-25T18:32:46Z

bors try

bors · 2020-06-25T19:20:39Z

try

Build failed:

ci/slurmci

charleskawczynski · 2020-07-21T14:09:25Z

bors r+

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 1315: Add VerticalIntegralModel for integrating flow and tendency r=blallen a=blallen # Description This PR adds a VerticalIntegralModel that can be used to integrate both the flow field and flow tendency for use in the split explicit model in #1268. This model is tested for correctness using the 2D and 3D hydrostatic spindown initial states. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com> Co-authored-by: Brandon Allen <ballen@mit.edu>

bors · 2020-07-21T15:51:31Z

Build failed (retrying...):

test-os (ubuntu-latest)

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 Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>

bors · 2020-07-21T17:35:11Z

Build failed:

test-os (macos-latest)

charleskawczynski · 2020-07-21T17:51:59Z

bors r+

@jm-c

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 · 2020-07-21T19:58:45Z

Build failed (retrying...):

ci/slurmci

charleskawczynski · 2020-07-21T22:37:07Z

bors r+

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 Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>

charleskawczynski · 2020-07-21T23:37:07Z

bors r+

bors · 2020-07-22T01:29:16Z

Build succeeded:

fill out communication functions and add necessary aux variables don't statecheck aux while adding new aux variables turn off restart values for now remove time averaging over fast mode and add flag for reconciliation add zero initializations replace .=- with .-= rebase fix for `test_vertical_integral_model.jl` convert `tendency_from_slow_to_fast!` to new reshape&broadcast setup Update `reconcile_from_fast_to_slow!` with new reshape&broadcast method Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

drive shallow water model by HB model add coupling traits, use to replace forcing traits in shallow water model move coupling for SWModel to file in SplitExplicit add coupling to HBModel, get fully coupled model to converge to analytic solution Add hydrostatic spindown tests with actual multi-rate happening remove old integral models get flow deviation working with new reshape&broadcast setup (on cpu). Remove stub functions for partial coupling use Coupling trait for dispatch instead of boolean in SplitExplicitMethod. use abbreviated model names Update to new functions from CliMA#1280

fill out communication functions and add necessary aux variables don't statecheck aux while adding new aux variables turn off restart values for now remove time averaging over fast mode and add flag for reconciliation add zero initializations replace .=- with .-= rebase fix for `test_vertical_integral_model.jl` convert `tendency_from_slow_to_fast!` to new reshape&broadcast setup Update `reconcile_from_fast_to_slow!` with new reshape&broadcast method Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

drive shallow water model by HB model add coupling traits, use to replace forcing traits in shallow water model move coupling for SWModel to file in SplitExplicit add coupling to HBModel, get fully coupled model to converge to analytic solution Add hydrostatic spindown tests with actual multi-rate happening remove old integral models get flow deviation working with new reshape&broadcast setup (on cpu). Remove stub functions for partial coupling use Coupling trait for dispatch instead of boolean in SplitExplicitMethod. use abbreviated model names Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

fill out communication functions and add necessary aux variables don't statecheck aux while adding new aux variables turn off restart values for now remove time averaging over fast mode and add flag for reconciliation add zero initializations replace .=- with .-= rebase fix for `test_vertical_integral_model.jl` convert `tendency_from_slow_to_fast!` to new reshape&broadcast setup Update `reconcile_from_fast_to_slow!` with new reshape&broadcast method Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

drive shallow water model by HB model add coupling traits, use to replace forcing traits in shallow water model move coupling for SWModel to file in SplitExplicit add coupling to HBModel, get fully coupled model to converge to analytic solution Add hydrostatic spindown tests with actual multi-rate happening remove old integral models get flow deviation working with new reshape&broadcast setup (on cpu). Remove stub functions for partial coupling use Coupling trait for dispatch instead of boolean in SplitExplicitMethod. use abbreviated model names Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

fill out communication functions and add necessary aux variables don't statecheck aux while adding new aux variables turn off restart values for now remove time averaging over fast mode and add flag for reconciliation add zero initializations replace .=- with .-= rebase fix for `test_vertical_integral_model.jl` convert `tendency_from_slow_to_fast!` to new reshape&broadcast setup Update `reconcile_from_fast_to_slow!` with new reshape&broadcast method Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

drive shallow water model by HB model add coupling traits, use to replace forcing traits in shallow water model move coupling for SWModel to file in SplitExplicit add coupling to HBModel, get fully coupled model to converge to analytic solution Add hydrostatic spindown tests with actual multi-rate happening remove old integral models get flow deviation working with new reshape&broadcast setup (on cpu). Remove stub functions for partial coupling use Coupling trait for dispatch instead of boolean in SplitExplicitMethod. use abbreviated model names Update to new functions from CliMA#1280 Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>

charleskawczynski added the Refactor Non-behavior changing label Jun 23, 2020

charleskawczynski requested review from blallen, simonbyrne, thomasgibson and jkozdon June 23, 2020 19:29

blallen reviewed Jun 23, 2020

View reviewed changes

experiments/OceanBoxGCM/homogeneous_box.jl Outdated Show resolved Hide resolved

charleskawczynski force-pushed the ck/refactor_bl branch 3 times, most recently from e87cd66 to a638fa8 Compare June 24, 2020 18:22

bors bot added a commit that referenced this pull request Jun 24, 2020

Try #1280:

12a458f

charleskawczynski force-pushed the ck/refactor_bl branch 2 times, most recently from 9697159 to 1dbcf7c Compare June 25, 2020 18:31

bors bot added a commit that referenced this pull request Jun 25, 2020

Try #1280:

dc054c0

charleskawczynski force-pushed the ck/refactor_bl branch 6 times, most recently from 066c21d to 74826a8 Compare June 25, 2020 23:52

charleskawczynski mentioned this pull request Jul 21, 2020

Unit tests may be leaking scope #1368

Open

3 tasks

charleskawczynski force-pushed the ck/refactor_bl branch from 7d3a582 to e6ca1a2 Compare July 21, 2020 17:51

charleskawczynski force-pushed the ck/refactor_bl branch from e6ca1a2 to ac09720 Compare July 21, 2020 21:55

Add types to BalanceLaw, and unify several funcs

7759e4b

charleskawczynski force-pushed the ck/refactor_bl branch from ac09720 to c43a2ce Compare July 21, 2020 22:35

Rename state_conservative -> state_prognostic

354affe

charleskawczynski force-pushed the ck/refactor_bl branch from c43a2ce to 354affe Compare July 21, 2020 22:36

bors bot merged commit dcf47cd into master Jul 22, 2020

bors bot deleted the ck/refactor_bl branch July 22, 2020 01:29

christophernhill mentioned this pull request Jul 27, 2020

Comments on #1280 style changes #1389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add types and unify functions in BalanceLaws #1280

Add types and unify functions in BalanceLaws #1280

charleskawczynski commented Jun 23, 2020 •

edited

Loading

blallen commented Jun 23, 2020

blallen commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

blallen commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020 •

edited

Loading

charleskawczynski commented Jun 24, 2020

bors bot commented Jun 24, 2020

charleskawczynski commented Jun 25, 2020

bors bot commented Jun 25, 2020

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 21, 2020

bors bot commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 22, 2020

Add types and unify functions in BalanceLaws #1280

Add types and unify functions in BalanceLaws #1280

Conversation

charleskawczynski commented Jun 23, 2020 • edited Loading

Description

For review by CLIMA developers

blallen commented Jun 23, 2020

blallen commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020

blallen commented Jun 23, 2020

charleskawczynski commented Jun 23, 2020 • edited Loading

charleskawczynski commented Jun 24, 2020

bors bot commented Jun 24, 2020

try

charleskawczynski commented Jun 25, 2020

bors bot commented Jun 25, 2020

try

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 21, 2020

bors bot commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

charleskawczynski commented Jul 21, 2020

bors bot commented Jul 22, 2020

charleskawczynski commented Jun 23, 2020 •

edited

Loading

charleskawczynski commented Jun 23, 2020 •

edited

Loading