Some touch-ups to porous barrier #186
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #186 +/- ##
============================================
+ Coverage 37.22% 37.26% +0.04%
============================================
Files 261 261
Lines 72340 72409 +69
Branches 13537 13532 -5
============================================
+ Hits 26929 26984 +55
- Misses 40417 40425 +8
- Partials 4994 5000 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
I agree that this PR adds some useful new capabilities and efficiencies for MOM6.
I have visually reviewed all of these changes, and they all look good to me. Although there are 12 substantive commits in this PR, each is a useful contribution in its own right, so they might we worth keeping, especially if this whole PR can synchronized with dev/gfdl via a rebase. Otherwise, some of the individual commits date back several months, so keeping the individual commits might not make sense for debugging any issues, in which case this PR should be dealt with via a squash merge.
I am approving this commit based on this visual review, conditional upon the updated TC testing and pipeline testing passing.
I would, however, be interested in any comments @sditkovsky might have on this PR.
|
@Hallberg-NOAA Thanks for the notification. I have rerun some of my test cases with these changes and the results look good to me. And I think it's great to have the new options for read-in method and interpolation schemes. @herrwang0 Thanks for the work. Glad the porous barriers functionality is coming in handy! |
|
@herrwang0 This is ready to be merged, but there are conflicts in some of the commits and it cannot be rebased. If you can be troubled to do a rebase ( Completely up to you, no pressure. |
* Porous barrier variables, which are grouped in porous_barrier_ptrs, are changed from pointers to allocatables. * Copies (aliases) of these variables in MOM_CS are removed. * The name porous_barrier_ptrs is yet to be changed to minimize the number of files needed to be edited.
* The interface porous_widths is deleted and subroutine por_widths is renamed as porous_widths. * porous_barrier_CS is added to control input and diagnostics of the module * Diagnostics for both interface and layer averages weights are added in subroutine porous_widths. * An _init subroutine is added to facilitate reading parameters and registering diagnostics * checksum debugs are added within subroutine porous_widths.
This commit primarily fixes indexing bugs in subroutine porous_widths. * Loop range is subroutine porous_widths is changed from data domain to computation domain. * find_eta call is now with a proper halo to cover all velocity points. * Halo update for porous barrier fields is added in step_MOM_*. Other changes: * mask_depth, a component of porous_barrier_CS is now used to as the criterion for masking. This removes the limit that the cell edge depth has to be below sea surface. * Output variable eta_cor was unused and is now changed to a local. * Some unused indexing variables are removed.
parameters * subroutine set_subgrid_topo_at_vel_from_file is added to read max, min and avg depth at the edges from file. * The subroutine is only called when a new runtime parameter SUBGRID_TOPO_AT_VEL is True. Default is false.
* id_clock is added (as a private variable) to time porous barrier calculations. * Some small format fixes
* A runtime parameter PORBAR_ETA_INTERP is added to control different methods for calculating interface height at the velocity points from adjacent tracer cells. * Two small thickness variables are added and scaled the unit of eta. This is to assit the harmonic mean calculation, but eventually the if-test eta_s - eta_prev>0 should be relaced by Angstrom. * Runtime parameter POROUS_BARRIER_MASKING_DEPTH is renamed to make it a liitlle bit shorter. * log_version call is added to separate out porous_barriers module in parameter file.
The main loops in porous_widths are simpified and cleaned up. * calc_por_layer iss slightly modified to reduced the if-blocks in the main loops. * k-loops are moved out. * To assist this new structure, two 2-D arrays are added to the stack. * A new function eta_at_uv is added to treat different interpolations. This is currently done at every point within the loop, and it is probably adding too much an overhead. It is better pre-calculate eta_[uv] all together, which would increase the stack size.
The averaged weight over a layer (por_face_area[UV]) and the weight at the interface (por_layer_width[UV]) are now calculated separately, as the only two updates in step_mom are in fact using only one of them. This reduces some unnecessary calculations. * Two new subroutines replaced the original `porous_widths`. They could be combined with interface if we remove porous_barrier_ptrs type, and use variables (of different nz) as output. * There is a place holder switch do_next in the two calc_por subs. This is used to further reduce calculations for all layers above D_max. Will be tested and implemented in the next commit. * A new subroutine calc_eta_at_uv is added to calculate eta at uv. This simplifies the code, but also increases stack and some performance overhead.
* A new runtime parameter USE_POROUS_BARRIER is added to control this feature. The default is True at the moment to assist potential test. It should be changed to false in the future. * A boolean array is added to avoid unnecessary porous barrier calculations above the shallowest points. * The layer-averaged weights are bounded by 1.0 explicitly, to avoid the cases it goes beyond due to roundoff errors. * For very thin layers (Angstrom), layer-averaged weights are set to zeros for simplicity. * Perhaps it is more reasonable to use the inferace weight for these cases. * It might be useful to make the thin layer definition a runtime parameter. * A bug related the size of eta_[uv] is fixed. This commit is potentially answer-changing when the porouss barrier module is used.
* The recently introduced ANSWER_DATE functionality is used to maintain a version that should be bit-for-bit reproducible for previous porous barrier related runs. * Rename porous_barrier_ptrs to porous_barrier_type * Some small documentation edits
* Fixed a bug that eta_[uv] was not properly initialized, which caused issues with global chksums with DEBUG=True. * Documents added to comply with Doxygen tests
herrwang0
force-pushed
the
porous_barrier_touchup
branch
from
89f239e to
9848f17
Compare
|
This appears to be failing in our static-memory builds in our CI. This is the backtrace: and the offending line is with the following error: I haven't yet probed further, but it is possible that one of these indices is not correctly being assigned its static-build index value. |
|
@herrwang0 I believe the issue is that the contents of MOM6/src/core/MOM_variables.F90 Lines 314 to 321 in a14c292 but that you are trying to allocate them with the static memory macro Lines 2490 to 2493 in a14c292 You will need to either use dynamic memory syntax for these fields, or re-declare the fields with the static memory macros ( (My preference is always for dynamic memory, but either is suitable.) |
|
Thanks @marshallward for catching this! The mix-up really exposes that I had no idea which was preferable! My understanding is that ALLOC_, ALLOCABLE_ ,NIMEM_, and etc defined in the memory macro cover both static and dynamic memory, is that right? Does that mean we should use this set? |
|
Compare your declarations to that of |
herrwang0
force-pushed
the
porous_barrier_touchup
branch
from
a14c292 to
bbc59f8
Compare
|
I decided to simply change it to the dynamic memory allocate. It might be more beneficial to switch these variables to subroutine input/output, but that could be a discussion in the future. |
Hi @adcroft , you were probably looking at the previous version of it real ALLOCABLE_, dimension(NIMEMB_PTR_,NJMEM_,NKMEM_) :: por_face_areaU !< fractional open area of U-faces [nondim]
real ALLOCABLE_, dimension(NIMEM_,NJMEMB_PTR_,NKMEM_) :: por_face_areaV !< fractional open area of V-faces [nondim]
real ALLOCABLE_, dimension(NIMEMB_PTR_,NJMEM_,NK_INTERFACE_) :: por_layer_widthU !< fractional open width
!! of U-faces [nondim]
real ALLOCABLE_, dimension(NIMEM_,NJMEMB_PTR_,NK_INTERFACE_) :: por_layer_widthV !< fractional open width
!! of V-faces [nondim]In this PR, this part is actually now gone and the grouped type (defined in MOM_variables) is used instead. Sorry it took this long to reply, I just realized what you were referring to just now (when I was solving the merge conflicts). |
|
@herrwang0 I have a rebased version of this PR, could you review it here? https://github.com/marshallward/MOM6/tree/pr_186_rebase For comparison: (We also might want to combine some of these commits, such as the bugfixes) |
|
@marshallward Thanks for doing the rebasing (I should have done that instead of merge before the latest bug fix)! |
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16574 ✔️ 🟡 The rebased version has passed, so I will merge that one. |
|
These commits have been merged in a rebased branch, so closing this PR. Note that the commits will have different hashes. |
This PR introduces some small changes to the porous barrier feature.
Edit: There are a number of small commits, maybe a squash commit should be done instead?
New features:
MOM_fixed_initializationto read global sub-grid scale topography parameters at velocity points.MOM_porous_barriersto manage relevant input parameters, diagnostics, and etc.Bug fixes:
eta_prevandA_layer_prevvalues, which was caused by a misplace of their updates inside an if-block. This is fixed after restructuring the corresponding code blocksPORBAR_MASKING_DEPTHto control this critical depth.Refactors:
MOM_variablesis change from pointer to allocatable.step_MOM, which in fact target the layers and interfaces separately. In the future, it might be helpful to use fortran interface to unify the subroutine names.do_nextmask is added to skip weight function calculation for all points above the shallowest point. This can make multi-layer runs (most layers in the open ocean are shallower than the topography) a bit more efficient.ANSWER_DATEflag is used to recover old results. (Question: the current value ofPORBAR_ANSWER_DATEis 20220806, should it be instead some future date when this PR is merged? )