-
Notifications
You must be signed in to change notification settings - Fork 55
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
Fix and cleanup vertical layer diagnostic #2837
Conversation
* Surface value for vertical integral was wrong * Ensure geopotential is indeed a potential * Use altitude for elevation above surface
* Rename altitude with height * Clean up includes * Clean up unit tests
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: bartgol |
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 # 5450 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Weaver # 5728 FAILED (click to see last 100 lines of console output)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting this fix in so quickly, and improving the functionality quite a bit. I had a few comments, but for the most part I think this is nearly ready to go in.
components/eamxx/src/diagnostics/tests/vertical_layer_tests.cpp
Outdated
Show resolved
Hide resolved
@@ -80,9 +80,9 @@ struct LongNames { | |||
std::map<std::string,std::string> name_2_longname = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to do away with this approach to long names altogether and just use the same syntax for hya
, hyb
, lev
, etc. as we do for the diagnostics? I.e. add longname as metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. But we can do it in a separate PR.
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: bartgol |
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 # 5454 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Weaver # 5732 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: bartgol |
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 # 5457 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Weaver # 5735 FAILED (click to see last 100 lines of console output)
|
Requesting field above sealevel requires phis, which this test doesn't have
@AaronDonahue I fixed the only issue left, the p3_standalone test. The baseline_cmp tests are expected to fail, since we fixed the bug in the vertical integral. If on the next AT run the only fails are the baseline_cmp tests, we're good to merge if you don't have any more comments. |
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: bartgol |
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 # 5458 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Weaver # 5736 FAILED (click to see last 100 lines of console output)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartgol , once you confirm the fails in the AT are the expected ones go ahead and manually merge.
The failures are the expected ones. Merging. |
A few mods were needed, since there was an inconsistency in the handling of the surface value, as well as on how this diagnostic was used in the FieldAtHeight diagnostic. Summary:
calculate_dz
works correctly, since it's already tested elsewhere. We just need to test that the diag computes the correct diagnostic type, and has the right behavior w.r.t. level index.Also, one thing we should do in all the diagnostics: we are not free to request a pack size. By the time diags are created, all fields have been allocated, so the diag does not have a saying in pack size. Instead, the diag should defer the creation of the output field until all inputs are set (i.e.., do it inside
initialize_impl
), and pick as a pack size for the output whatever can work with all its inputs (i.e., the smallest among the pack sizes of the inputs).