-
Notifications
You must be signed in to change notification settings - Fork 75
Extend diag_mediator to allow the piecemeal posting of diagnostics #809
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
base: dev/gfdl
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #809 +/- ##
===========================================
Coverage ? 21.81%
===========================================
Files ? 137
Lines ? 33019
Branches ? 5850
===========================================
Hits ? 7202
Misses ? 25260
Partials ? 557 ☔ View full report in Codecov by Sentry. |
Hallberg-NOAA
left a comment
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.
I think that this capability should be extended to include the ability to initialize a diagnostic buffer to land values to avoid the need to call the piecemeal posting for all columns, even over land.
Also, this PR should be revised to adhere to the guidance in the MOM6 style guide at https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide, as described in specific comments.
|
@Hallberg-NOAA Thank you for the comments. I have added descriptions of the units (see the inline response) and made the if-statements conform the to the style guide. Additionally, the fill value for the arrays can now be specified; for the purposes of the piecemeal posting of the diagnostics these are set to the |
b892a0d to
bc2a91f
Compare
8380b62 to
2b83320
Compare
Hallberg-NOAA
left a comment
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.
With this latest set of changes, all of my concerns have been addressed, and I think that this PR is now ready to be merged in.
2b83320 to
4eebe2f
Compare
Hallberg-NOAA
left a comment
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.
Please modify the syntax of this code so that it compiles and works correctly with all 3 of the intel, gnu and nvidia compilers.
Some quantities in MOM6 are calculated in subroutines which expect slices of the model's 2d or 3d arrays. Diagnosing these quantities can be challenging because the usual post_data calls expect whole arrays. To solve this problem, the changes here introduce a dynamic buffer that can grow as needed over the course of an simulation. This buffer keeps track of the diagnostic IDs and the index in the buffer. When a slot in the buffer is no longer needed, for example if the whole array has been computed and posted, the slot is marked as "available" for overwriting. The buffer is only allowed to grow if all the currently allocated slots are in use. Any computational cost associated with growing this buffer will only happen in the first few steps of the model as post_data for requested diagnostics is called for the first time in that run.
The diag mediator has been extended to add a dynamic buffer to each axes group. Three new methods have also been added to enable the piecemeal posting (by column, by point) of a diagnostic and a 'final' method to allow the buffer to be reused later.
ePBL calculates the vertical diffusivity column by column. This provides a convenient sanity check of the new piecemeal posting of diagnostics. The original diagnostic Kd_ePBL is done by posting the full 3d prognostic array, whereas a new diagnostic Kd_ePBL_col_by_col posts the same array from within ePBL but does so column by column.
Fixes failures in the CI due to some procedures and type members not having docstrings.
4eebe2f to
3891e33
Compare
|
Unfortunately we are now getting segmentation faults with all three of the intel, gnu and nvidia compilers when we try to run the pipeline testing with this PR. The GFDL regression testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/28215 (and in particular the intel run job at This did pass our TC testing, but I triggered the test again a few hours later and confirmed that these problems are not arising because of a temporary problem with the Gaea computer. @ashao, if you get the chance to revisit this, that would be appreciated. |
|
Just to clarify is this a new regression for this branch, or had this branch not previously been run on the internal pipeline before? |
|
I went back and looked at the original regression testing that we did back in late February at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26532. That version did not compile with the Nvidia compiler, but it did compile with gnu and intel. When it ran, it was also getting segmentation faults and bus errors. We focused on the compile failures, but the run-time errors were also problematic, and we should have pointed these out at the same time. The failed tests include everything using ePBL, the simplest of which would be something like ocean_only/single_column/EPBL. |
|
@Hallberg-NOAA: Thanks! That will definitely save some time in the debugging. |
Some quantities in MOM6 are calculated in subroutines which expect slices of the model's 2d or 3d arrays. Diagnosing these quantities can be challenging because the usual post_data calls expect whole arrays. To solve this problem, the changes here introduce a dynamic buffer that can grow as needed over the course of an simulation.
This buffer keeps track of the diagnostic IDs and the index in the buffer. When a slot in the buffer is no longer needed, for example if the whole array has been computed and posted, the slot is marked as "available" for overwriting. The buffer is only allowed to grow if all the currently allocated slots are in use.
Any computational cost associated with growing this buffer will only happen in the first few steps of the model as post_data for requested diagnostics is called for the first time in that run.
This new piecemeal posting of a diagnostic was implemented for
Kd_ePBL. The new diagnosticKd_ePBL_col_by_colis the same as the the original field in theBaltic_OM4_05case.@adcroft @marshallward: Two points in particular that I'd like your feedback on:
post_datacall to be done without an extra copy. I have included astoresubroutine that could be used if we made it private, but otherwise those should be deleted.