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

Move first order moisture flux to new tendency specification #1725

Merged
merged 1 commit into from
Nov 21, 2020

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Nov 9, 2020

Description

This PR

  • moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for experiments/AtmosLES/bomex_model.jl with moisture_model == "nonequilibrium", but I haven't checked the results.
  • Renames flux_moisture! to flux_first_order! in efforts to standardize some of the AtmosModel method calls
  • Adds Moisture <: PrognosticVariable to combine specification for the moisture variables.
  • 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.

Copy link
Contributor

@kpamnany kpamnany left a comment

Choose a reason for hiding this comment

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

it seems that NonEquilMoist models are not tested

Maybe add bomex_les.jl --moisture-model=nonequilibrium also to the pipeline?

src/Atmos/Model/AtmosModel.jl Outdated Show resolved Hide resolved
src/Atmos/Model/atmos_tendencies.jl Outdated Show resolved Hide resolved
@charleskawczynski
Copy link
Member Author

Maybe add bomex_les.jl --moisture-model=nonequilibrium also to the pipeline?

I'll add the single stack one, since this is cheaper but still exercises the kernels.

@charleskawczynski charleskawczynski force-pushed the ck/tend_f1_moisture branch 4 times, most recently from 4279a09 to a42811b Compare November 9, 2020 16:54
@trontrytel
Copy link
Member

it seems that NonEquilMoist models are not tested

Maybe add bomex_les.jl --moisture-model=nonequilibrium also to the pipeline?

The plan was to have a full bomex run with both equilibrium and nonequilibrium mositure as a part of weekly/nightly tests. Once we have them

@trontrytel
Copy link
Member

In general I would say that moisture variables (q_tot, q_liq and q_ice) will have the same flux but different source terms. And then the upcoming precipitation variables (q_rai and q_sno) will have different fluxes and source terms.

@charleskawczynski
Copy link
Member Author

In general I would say that moisture variables (q_tot, q_liq and q_ice) will have the same flux but different source terms. And then the upcoming precipitation variables (q_rai and q_sno) will have different fluxes and source terms.

That's great, we can group them as we need, then.

@charleskawczynski
Copy link
Member Author

The plan was to have a full bomex run with both equilibrium and nonequilibrium mositure as a part of weekly/nightly tests. Once we have them

Sounds good. I'll leave it for now so that we know that the kernels are syntactically correct, but I agree that we should hook in QA tests.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Unfortunately my limited understanding of the balance law framework hinders my ability to review this PR. Nothing egregious pops out to me so I'm approving.

A higher-level question is --- does it make sense to modularize the microphysics implementation a bit more? Right now it looks like terms like "mass, momentum, energy" prognostic variables are specified alongside moisture terms. A more modular design might separate these implementations a bit more (I can't tell if they just happen to be nearby in the code, or if their proximity reflects a deeper entanglement).

Can you use independent balance laws to different aspects of the equations (one for "primary" variables like mass, momentum, and energy, another for microphysics variables, another for passive chemical species) to enforce / ease this modularization?

@charleskawczynski
Copy link
Member Author

A higher-level question is --- does it make sense to modularize the microphysics implementation a bit more? Right now it looks like terms like "mass, momentum, energy" prognostic variables are specified alongside moisture terms. A more modular design might separate these implementations a bit more (I can't tell if they just happen to be nearby in the code, or if their proximity reflects a deeper entanglement). Can you use independent balance laws to different aspects of the equations (one for "primary" variables like mass, momentum, and energy, another for microphysics variables, another for passive chemical species) to enforce / ease this modularization?

They are modularized in the sense that we have fallback methods for (for example) flux_first_order!(atmos.moisture, ...) which do nothing when atmos.moisture isa DryModel, and do moisture / microphysics things for moist models.

@glwagner
Copy link
Member

They are modularized in the sense that we have fallback methods for (for example) flux_first_order!(atmos.moisture, ...) which do nothing when atmos.moisture isa DryModel, and do moisture / microphysics things for moist models.

For sure, my questions is about "more" modularization (not necessarily functionally but just by writing the code differently, eg by using different files), which makes it easier for future developers to build further without impacting the readability of core stuff.

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Nov 19, 2020
1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 19, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Nov 19, 2020
1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 19, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Nov 20, 2020
1687: Extract remaining citations r=charleskawczynski a=charleskawczynski

### Description

Extracts remaining references/citations to `docs/bibliography.bib`.



1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 20, 2020

Build failed (retrying...):

bors bot added a commit that referenced this pull request Nov 20, 2020
1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 20, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Nov 20, 2020
1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 20, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Nov 21, 2020
1725: Move first order moisture flux to new tendency specification r=charleskawczynski a=charleskawczynski

### Description

This PR

 - moves the first order moisture flux to the new tendency specification. I can confirm that the tendency table is updated, for `experiments/AtmosLES/bomex_model.jl` with `moisture_model == "nonequilibrium"`, but I haven't checked the results.
 - Renames `flux_moisture!` to `flux_first_order!` in efforts to standardize some of the AtmosModel method calls
 - Adds `Moisture <: PrognosticVariable` to combine specification for the moisture variables.




Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 21, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 21, 2020

@bors bors bot merged commit 673a30c into master Nov 21, 2020
@bors bors bot deleted the ck/tend_f1_moisture branch November 21, 2020 03:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Atmos Atmosphere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants