Dev gfdl main candidate 2021 10 04 #1507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge dev gfdl 19may2021
This is required for the u,v sponges to be invariant of tiling. I don't know why, but the problem only showed up for me in a narrow channel in the Bering domain.
The MOM_control_struct is declared and passed as a pointer to a CS (usually allocated anonymously) and treated as if it were a pointer, even though there is currently no real advantage to doing so. After the FMS update, the deallocation of this CS was causing a segmentation fault in the PGI compilers. While the underlying cause was never determined, it is likely due to some automated deallocation of the CS contents, whose addressing became scrambled. This problem can be resolved by moving all of the CS contents to stack, so that the contents are automatically removed upon exiting whatever function it was instantiated. Subsequent calls can reference the local (or parent) stack contents.
The MOM_control_struct is declared and passed as a pointer to a CS (usually allocated anonymously) and treated as if it were a pointer, even though there is currently no real advantage to doing so. After the FMS update, the deallocation of this CS was causing a segmentation fault in the PGI compilers. While the underlying cause was never determined, it is likely due to some automated deallocation of the CS contents, whose addressing became scrambled. This problem can be resolved by moving all of the CS contents to stack, so that the contents are automatically removed upon exiting whatever function it was instantiated. Subsequent calls can reference the local (or parent) stack contents.
…into ice_dynamics
This patch introduces two new testing targets to the verification suite based on a small configuration based on the `benchmark` regression test. The profile test is saved a `p0` in `.testing`. Future tests can be included if appropriate. The new targets: * `make profile`: Run the model and record the FMS timings. * `make perf`: Run the model through the `perf` tool and record timings for the resolvable functions (as symbols). In both cases, the timings are converted to JSON output files and the top results are reported to stdout, and readable in GitHub actions output. It can also be run locally. Support Python scripts have been included to do this work. This will require a functional Python environment. Some system and configuration data is logged alongside the timings, but this is still rather incomplete and needs some further planning. Times are compared to the target build (usually dev/gfdl). ANSI terminal highlighting (i.e. color) is to used to highlight excessive differences. Current issues: - Model configuration - GitHub timings are still rather unreliable, and should currently only be treated as crude estimates. This should be considered a work in progress. - The GitHub profiling rule still builds the standard configuration, evem though it is unused. - Additional tools are required to push the timings to some database, either a local sqlite3 or an external one.
- Without this change, the edges don't reproduce on restart due to the h values outside being nonsense.
Added new diagnostics of the acceleration driven by the wind stress accelerations as redistributed by a timestep of the vertical viscosity and not lost to bottom drag within a timestep. This is also in the diagnostics of the accelerations due to the vertical viscosity, but the redistribution can be found from the difference of the two. Also added a diagnostic of the contribution of the wind stresses to kinetic energy, and applied an underflow limiter on both the new acceleration diagnostic and the existing viscous acceleration diagnostic. All solutions are bitwise identical, but there are new diagnostics.
Add checks for inconsistent parameter settings in adiabatic_driver_init() when ADIABATIC = True, and issue instructive error messages if any are found. This PR addresses MOM6 issue #1417. All answers are bitwise identical, although some cases where the inconsistent parameter settings were previously ignored may now issue fatal errors and will not run.
Added code to lock the restart registry once all registration should have occurred or if the restart has been read, along with a new public interface, restart_registry_lock, to allow this lock to be set or unset. All calls to register restart fields now check the state of this lock and issue a fatal error if the registry is locked. This PR addresses MOM6 issue #1214. In the process of adding this restart lock, the new error messages revealed that some of the restart registration calls related to some types of open boundary conditions were not happening early enough. To avoid this, a new interface, register_DOME_OBC, was added to the DOME_initialization module and is being called from call_OBC_register, and a number of the OBC-related calls during the initialization were collected in the same (appropriate) place. Some OBC error messages were also corrected. All answers are bitwise identical, but there are two new public interfaces and the order of some OBC-related entries in the MOM_parameter_doc calls changed.
- This addresses the FMS issue $761 NOAA-GFDL/FMS#761 - There is a mpp_broadcast in the FMS2 subroutine get_unlimited_dimension_name() and this subroutine has to be called by all pes, so it cannot be inside a if(is_root_pe()) block
Added a test to avoid attempting to deallocate the geothermal heating field if it is not allocated, and changed the geo_heat element of geothermal_CS from a pointer into an allocatable array. Also clarified the comments describing several of the elements of geothermal_CS, and added a test to avoid logging the value of GEOTHERMAL_DRHO_DT_INPLACE when the model is not in layered-mode and this parameter is unused. This PR addresses MOM6 issue #1449. All answers are bitwise identical in all cases that worked before, but there are fewer entries in some ALE-mode MOM_parameter_doc files.
|
Approved. All CESM tests pass with expected answer changes due to MEKE updates (6979c29) |
|
works fine in UFS except a new variable "First_direction" is being added in restart file which made cmp comparison between UFS current baseline results and restart files from this updated code failed. Except this new variable, all other common variables remain the same. |
|
@jiandewang Is the |
|
@marshallward no this won't be an issue for UFS, all we need to do is re-generating UFS baseline. |
klindsay28
reviewed
Oct 8, 2021
| @@ -260,7 +267,8 @@ subroutine call_tracer_register(HI, GV, US, param_file, CS, tr_Reg, restart_CS) | |||
| if (CS%use_dyed_obc_tracer) CS%use_dyed_obc_tracer = & | |||
| register_dyed_obc_tracer(HI, GV, param_file, CS%dyed_obc_tracer_CSp, & | |||
| tr_Reg, restart_CS) | |||
|
|
|||
| if (CS%use_nw2_tracers) CS%use_ideal_age = & | |||
There was a problem hiding this comment.
Is this line correct? I suspect the delta should be
+ if (CS%use_nw2_tracers) CS%use_nw2_tracers =
There was a problem hiding this comment.
Fortunately, all this shoudl do is turn on ideal_age in addition to NW2, without breaking the NW2 tracers. Good catch - thanks. Currently testing a patch...
|
COAPS approves this PR |
klindsay28
reviewed
Oct 8, 2021
| @@ -88,6 +90,7 @@ module MOM_tracer_flow_control | |||
| logical :: use_pseudo_salt_tracer = .false. !< If true, use the psuedo_salt tracer package | |||
| logical :: use_boundary_impulse_tracer = .false. !< If true, use the boundary impulse tracer package | |||
| logical :: use_dyed_obc_tracer = .false. !< If true, use the dyed OBC tracer package | |||
| logical :: use_nw2_tracers = .false. !< If true, use the ideal age tracer package | |||
There was a problem hiding this comment.
The comment on this line should probably be referring to the nw2 (or NeverWorld2) tracer package, not ideal age.
There was a problem hiding this comment.
Yes! This and the comment above both are due to copying the ideal tracer module as a starting point. Good catch - we'll clean it up.
…-10-04 correct long_name for tracer_dfy for passive tracers when diag_form == 1
|
A documentation update (#1513) was provided by @klindsay28 ; this should not require extra review. A reproducibility issue was discovered by @adcroft related to topography (#1428). It may be related to the unresolved issue which arose after that PR was merged. Alistair is investigating this, and we will resolve this issue before this PR is accepted to |
PRs #1428 and #1457 extended the topography clipping to allow flooding but missed the use case for positive-only depths where the MASKING_DEPTH parameter alone was in use. There were two bugs: 1. The new code assumed that MINIMUM_DEPTH would be deeper than MASKING_DEPTH (which is intuitive). However, the point of MASKING_DEPTH was only to specify the determination of the land mask. The new code assigned depths the value of MASKING_DEPTH which broke cases that were using MASKING_DEPTH as documented and were leaving MINIMUM_DEPTH=0. 2. The values of variable masking_depth were altered and subsequently not consistent with the logged parameters. A warning was issued but the behavior was nevertheless not as intended. Changes: 1. Removed the test that masking_depth > min_depth, and warning 2. Adjusted the condition and assigned value when clipping depths. This now uses the shallower of min_depth and masking_depth to decide when to clip and for the value to use otherwise. The expression for the land mask is unaltered. 3. Corrected documentation to retain original purpose of MASKING_DEPTH 4. Added some comments for declaration with units 5. Added some clarifying comments in code Todo: - resolve the need for the alternative negative depth pathway associated with the 0.5*min_depth expression.
- @klindsay28 spotted two issues for the NW2 tracers 1. Use of the wrong logical variable 2. Incorrect comment - Using the wrong logical meant that the ideal age pacakge was being called in addition to the NW2 package, but did not affect the NW2 tracers themselves.
- Following feedback from @herrwang0, we have removed the possibility for a user to try using negative depths without the MASKING_DEPTH parameter being set appropriately. This avoids the asymmetric use of MINIMUM_DEPTH that was proposed. A FATAL is now issued. - Corrected a spelling error in a comment. - Removed an unused "use" that should have been done in previous commit.
Recover topography clipping when not specifying MINIMUM_DEPTH
sanAkel
approved these changes
Oct 13, 2021
There was a problem hiding this comment.
All seems okay with this PR; tested including latest commits.
Correct logical associated with NW2 tracers
|
There has been one major update to the PR related to topography initialization, as well as two minor updates, so I think it would be best if everyone review the changes again. |
|
I re-ran UFS with Marshall's latest commit NOAA-GFDL@ecf88c6, no change compared with the original PR code |
|
I approve this PR. |
|
Same here, all okay! |
|
Still good for COAPS. |
|
approved again. all CESM tests passing with answer changes. |
|
All approved, thanks to everyone for their work. |
gustavo-marques
added a commit
to NCAR/MOM6
that referenced
this pull request
Oct 25, 2021
Main to dev/ncar (after mom-ocean#1507 merge)
This was referenced Nov 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the following major changes:
G%Z_ref.read_data.Review
Features
Bugfixes
Refactor
Support
Contributors: