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.

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

add remaining diagnostics to the new diagnostics package #2200

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

szy21
Copy link
Member

@szy21 szy21 commented Oct 5, 2023

Purpose

Add EDMFX diagnostics and add some default diagnostics when running simulations with EDMFX. I think this includes all the important diagnostics we want to have.

To-do

  • Check if the diagnostics are physically correct

Content


  • I have read and checked the items on the review checklist.

@szy21 szy21 added the do-not-merge-yet Block from bors label Oct 5, 2023
@szy21 szy21 added this to the O3.2.2 Add a diagnostic module milestone Oct 5, 2023
@szy21 szy21 self-assigned this Oct 5, 2023
@szy21 szy21 marked this pull request as ready for review October 6, 2023 04:47
@szy21 szy21 requested a review from Sbozzolo October 6, 2023 04:47
@szy21 szy21 removed the do-not-merge-yet Block from bors label Oct 6, 2023
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

Looks good!

My main comment (other than the tiny details below) is that you the "comment" field in your variables is not really useful since you just paraphrase the long_name.

Maybe we can copy and paste the comments from the airtable with all the CMIP quantities?

config/model_configs/diagnostic_edmfx_bomex_box.yml Outdated Show resolved Hide resolved
src/diagnostics/default_diagnostics.jl Outdated Show resolved Hide resolved
src/diagnostics/default_diagnostics.jl Outdated Show resolved Hide resolved
@szy21
Copy link
Member Author

szy21 commented Oct 6, 2023

Maybe we can copy and paste the comments from the airtable with all the CMIP quantities?

All the EDMFX specific diagnostics don't have a corresponding variable in the CMIP table, as they don't use the same convection scheme in CMIP. The comments for all updraft variables (diagnostics ending with up) are mostly to just describe that the diagnostics is for the first updraft. The comments for environment variables are basically the long_name, maybe I can just remove them?

@Sbozzolo
Copy link
Member

Sbozzolo commented Oct 6, 2023

Maybe we can copy and paste the comments from the airtable with all the CMIP quantities?

All the EDMFX specific diagnostics don't have a corresponding variable in the CMIP table, as they don't use the same convection scheme in CMIP. The comments for all updraft variables (diagnostics ending with up) are mostly to just describe that the diagnostics is for the first updraft. The comments for environment variables are basically the long_name, maybe I can just remove them?

When I look the table, I see for example for cli:

Includes both large-scale and convective cloud. Calculate as the mass of cloud liquid water in the grid cell divided by the mass of air (including the water in all phases) in the grid cells. Precipitating hydrometeors are included ONLY if the precipitating hydrometeors affect the calculation of radiative transfer in model.

I don't know how much detail we should have, but I think that having more is better than having less (especially if the variable is never going to change).

@szy21
Copy link
Member Author

szy21 commented Oct 6, 2023

When I look the table, I see for example for cli:

Includes both large-scale and convective cloud. Calculate as the mass of cloud liquid water in the grid cell divided by the mass of air (including the water in all phases) in the grid cells. Precipitating hydrometeors are included ONLY if the precipitating hydrometeors affect the calculation of radiative transfer in model.

I don't know how much detail we should have, but I think that having more is better than having less (especially if the variable is never going to change).

Oh, right, I thought you meant the EDMFX variables. Yes, I can add more comments to variables like clw and cli. One reason I didn't add it is that the comments from the table don't apply to our model yet. For example, for cli, we don't have convective cloud, or precipitating hydrometeors. But maybe we can just add the comments and change how the diagnostics are calculated later?

@Sbozzolo
Copy link
Member

Sbozzolo commented Oct 6, 2023

When I look the table, I see for example for cli:

Includes both large-scale and convective cloud. Calculate as the mass of cloud liquid water in the grid cell divided by the mass of air (including the water in all phases) in the grid cells. Precipitating hydrometeors are included ONLY if the precipitating hydrometeors affect the calculation of radiative transfer in model.

I don't know how much detail we should have, but I think that having more is better than having less (especially if the variable is never going to change).

Oh, right, I thought you meant the EDMFX variables. Yes, I can add more comments to variables like clw and cli. One reason I didn't add it is that the comments from the table don't apply to our model yet. For example, for cli, we don't have convective cloud, or precipitating hydrometeors. But maybe we can just add the comments and change how the diagnostics are calculated later?

I think we should describe what we have, and update the description when we update the diagnostic.

@szy21 szy21 force-pushed the zs/add_edmf_diagnostics branch 4 times, most recently from 187b8c7 to 16aea10 Compare October 11, 2023 21:14
@szy21
Copy link
Member Author

szy21 commented Oct 11, 2023

@Sbozzolo I updated the comment of several variables following the CMIP air table, and removed the comment of some variables where they are similar to the long name. I also set the default EDMF diagnostics to daily average, and included an example yaml file in ci that calculates 10-min instantaneous EDMF diagnostics. The results look physical. Would you like to take another look?

@szy21 szy21 force-pushed the zs/add_edmf_diagnostics branch from 16aea10 to 77c79e7 Compare October 11, 2023 21:19
@Sbozzolo
Copy link
Member

@Sbozzolo I updated the comment of several variables following the CMIP air table, and removed the comment of some variables where they are similar to the long name. I also set the default EDMF diagnostics to daily average, and included an example yaml file in ci that calculates 10-min instantaneous EDMF diagnostics. The results look physical. Would you like to take another look?

It looks good to me. The only thing: could you split the extremely long strings to multiline strings?

Also, I didn't realize how many EDMF examples we are running. In the future, if we want the whole suite of diagnostics for all the EMDF examples, we will probably have to define them as default in the code. (But this is good for now).

@szy21
Copy link
Member Author

szy21 commented Oct 11, 2023

It looks good to me. The only thing: could you split the extremely long strings to multiline strings?

I changed some of the comment to multiline strings. Is that what you mean?

Also, I didn't realize how many EDMF examples we are running. In the future, if we want the whole suite of diagnostics for all the EMDF examples, we will probably have to define them as default in the code. (But this is good for now).

Yeah, I think it would be good if we don't need to specify them for each job. I wonder if the config can take multiple yaml files now (it could before I think). If so, we can have one common diagnostic yaml file for the current edmf examples.

@szy21 szy21 force-pushed the zs/add_edmf_diagnostics branch from b052c48 to 7dfaba4 Compare October 12, 2023 00:02
@szy21
Copy link
Member Author

szy21 commented Oct 12, 2023

bors r+

bors bot added a commit that referenced this pull request Oct 12, 2023
2200: add remaining diagnostics to the new diagnostics package r=szy21 a=szy21



Co-authored-by: Zhaoyi Shen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 12, 2023

Build failed:

@szy21
Copy link
Member Author

szy21 commented Oct 12, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 12, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit baa37f0 into main Oct 12, 2023
6 checks passed
@bors bors bot deleted the zs/add_edmf_diagnostics branch October 12, 2023 03:08
@simonbyrne
Copy link
Member

simonbyrne commented Oct 12, 2023

Part of #2198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants