pbl entrainment budget diags #2822
Conversation
mahf708
added
the
AT: WIP
|
|
||
| auto ws = wsm.get_workspace(team); | ||
| ekat::Unmanaged<WSMgr::view_1d<Real>> tm_grad; | ||
| ws.take_many_contiguous_unsafe<wsms>({"tm_grad"}, {&tm_grad}); |
There was a problem hiding this comment.
It's not wrong, but since you use only one slot, you can prob just use auto tm_grad = ws.take("tm_grad").
There was a problem hiding this comment.
I do want to keep the full wsm stuff around because this is not the final place these diags will be in. I will likely add at least a few more temp views here soon
| team.team_barrier(); | ||
|
|
||
| // release stuff from wsm | ||
| ws.release_many_contiguous<wsms>({&tm_grad}); |
There was a problem hiding this comment.
If you switch to take, then this can become a simple release()
| size = 0; | ||
|
|
||
| // pblp | ||
| index_map["p+"] = size++; |
There was a problem hiding this comment.
What are + and ^ in the diags names?
There was a problem hiding this comment.
+: above the inversion (so at min_tm_grad_lev-1 in the algo)
^: integrated under the inversion (so integrated from num_levs to min_tm_grad_lev)
There was a problem hiding this comment.
Are these "common" names in this context? If not, I would rather use some more wordy names...
There was a problem hiding this comment.
They kind of are defined like this. I mean, that's how they're defined in the reference eqs I used. But, we don't have to follow what these eqs do and we can do our own more verbose descriptions
There was a problem hiding this comment.
(I may also decide to add technical docs with this PR either now or later...)
mahf708
force-pushed
the
mahf708/diags/extra-swaci-diags
branch
from
9555a54
to
8e42850
Compare
mahf708
force-pushed
the
mahf708/diags/extra-swaci-diags
branch
from
8e42850
to
f445669
Compare
mahf708
added
priority:low
diagnostic
and removed
AT: WIP
mahf708
force-pushed
the
mahf708/diags/extra-swaci-diags
branch
from
f445669
to
be0c10e
Compare
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5422 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 5702 FAILED (click to see last 100 lines of console output) |
E3SM-Autotester
removed
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5424 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 5704 FAILED (click to see last 100 lines of console output) |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
|
E3SM-Autotester
removed
the
AT: RETEST
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, target_sha=5555fa2c22ee08aa2ac6b17c06968c9858d09387, However Inspection must be performed before merge can occur... |
mahf708
added
the
AT: WIP
|
What is the status of this? It's labeled WIP, is it a part of the output fails you are trying to debug? |
No, it is not part of debugging (the fail happens with/without it). I made it WIP because I didn't want it to keep testing. I decided to use an earlier commit in my runs (so I will cherry-pick it and there is no rush to get this integrated) Do you think we should merge it? |
|
If you aren't using this version of the diag (and no one else is needing it) then I guess we should leave it unmerged. Re-request reviews if/when you want to merge it. |
I do want to use it, so let me rerequest reviews right now and take out the WIP label |
mahf708
removed
the
AT: WIP
|
All Jobs Finished; status = PASSED, target_sha=5555fa2c22ee08aa2ac6b17c06968c9858d09387, However Inspection must be performed before merge can occur... |
There was a problem hiding this comment.
I don't see any mods to scorpio_output.cpp. The user can still request it using PBLEntrainmentBudget as field in the output yaml, but there's no control over the options. If you will ever want to support the other pblinvalg options, you will need to have multiple strings to map to that diag, which means that scorpio_output.cpp will have to handle them, setting the proper values for the diag parameter list.
I'm assuming for now you're just interested in the default temperature-inversion case. Perhaps add a comment in the header file, explaining that the only supported case is temperature-inversion for now?
| // Get pseudo_density | ||
| add_field<Required>("pseudo_density", scalar2d_layout, Pa, grid_name); | ||
| // Get radiation up and down terms | ||
| add_field<Required>("SW_flux_dn", scalar2d_layout, W / m * m, grid_name); |
There was a problem hiding this comment.
Watch out, these units here will resolve to W, since you don't have parentheses!
Note: this will have zero impact on this code working, since I/O does not check the units of the requested fields. But we should fix this, a) to avoid confusion and b) in case I/O starts to check units of the diags required fields.
| m_units_map = eadu.units_map; | ||
| m_ndiag = eadu.size; | ||
|
|
||
| if(eadu.pblinvalg == "temperature-inversion") { |
There was a problem hiding this comment.
This is the only possible scenario, since eadu is default-constructed, so it will always have "temperature-inversion" as pblinvalg. In the long run, I'm guessing this code should change into something like
PBLEntrainmentBudgetDiagUtil eadu;
m_index_map = eadu.index_map;
m_units_map = eadu.units_map;
m_ndiag = eadu.size;
eadu.pblinvalg = m_params.get<std::string>("pblinvalg");
if (eadu.pblinvalg=="temperature-inversion") {
m_pblinvalg = 0;
} else if (...) {
} else {
EKAT_ERROR_MSG ("Watch out, silly: invalid value for pblinvalg!\n");
}There was a problem hiding this comment.
That's right...
So, I don't know what to do here. On the one hand, I already implemented three different methods to do this below (all rely on "state inversion" --- an inversion in the gradient of one "state" variable, namely temperature, liquid potential temperature, or water content. There are other more weird ways to determine this, including relationships in derived fluxes and so on, which I could also implement, but my thinking is, we should likely stick to state variables.
The reason I didn't implement this like AeroComCldTop/Bot is because.... I don't know what to name the variables yet 😄 PBLEntrainmentBudgetTemperature? PBLEntrainmentBudget_t? PBLEntrainmentBudget1? Do you have ideas? :D
There was a problem hiding this comment.
I have no clue what these variables represent, so I can't advise on this. But it's confusing to have code that is guaranteed to never be executed. I would pick some name for those diagnostics, and just do the mods to support them all. Alternatively, remove all other implementations, and hard-code the version for "temperature-inversion", without giving the idea that this is a configurable option...
There was a problem hiding this comment.
Is it okay with you if I do PBLEntrainmentBudget0, PBLEntrainmentBudget1, etc.?
There was a problem hiding this comment.
Ugh, if there's no other way, then ok. But I would prefer "Temperature" or "T", as you suggested. The name length is already super long, so no point in being cheap (and cryptic) with the last word...
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5449 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 5727 FAILED (click to see last 100 lines of console output) |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5456 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 5734 PASSED (click to see last 100 lines of console output) |
E3SM-Autotester
removed
the
AT: RETEST
mahf708
added
the
AT: WIP
|
WIP status: I will try to merge this after upstream merge to bring kokkos 4.2. The PR doesn't need kokkos 4.2, but I can edit some parts in it to take advantage of kokkos 4.2 |
|
Update: Lost motivation/drive to finish this PR. Closing. People interested in these diagnostics can try to incorporate them. I will revisit later when I have more clarity and bandwidth. |
adding pbl entrainment budget diags to eamxx