Fixes issues with the GME code and get_param calls for Leith options #65
Conversation
Added do_not_log arguments to get_param calls in MOM_hor_visc.F90 that are only used conditionally, and eliminated the unlogged GET_ALL_PARAMS runtime parameter and get_all variable in hor_visc_init(). By design, all logging of parameters after this commit is identical to before, even for variables that are inactive and therefore should not be logged. In several places, there were some problems, mostly with the GME code, that have been noted in comments marked with '###'. Also cleaned up the code alignment and eliminated unneeded temporary variables in a few places in hor_visc(). All solutions are bitwise identical, and no output is changed.
The call to get the 3-d GME diffusivity arrays and the subsequent blocking halo update was moved outside of the k-loop.
* Makes the GME filter rotationally invariant * Adds a runtime param to allow the user to control how many smoothing passes should be done.
The get_param calls for Leith were not in the correct location. This commit fixes that.
* Adds option to save barotropic tension and strain; * Fixes many i and j loops indices associated with GME to avoid halo problems and unnecessary halo updates. With these changes, the model is now conserving mass and tracers when USE_GME = True.
The calculation now mimics the calculation of FrictWork and it includes the energy diffusion term.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #65 +/- ##
============================================
- Coverage 29.02% 29.00% -0.02%
============================================
Files 244 244
Lines 71854 71880 +26
============================================
- Hits 20854 20851 -3
- Misses 51000 51029 +29
Continue to review full report at Codecov.
|
| do j=Jsq-1,Jeq+2 ; do i=Isq-1,Ieq+2 | ||
| boundary_mask_h(i,j) = (G%mask2dCu(I,j) * G%mask2dCv(i,J) * G%mask2dCu(I-1,j) * G%mask2dCv(i,J-1)) | ||
| enddo ; enddo | ||
|
|
||
| do J=js-2,Jeq+1 ; do I=is-2,Ieq+1 | ||
| boundary_mask_q(I,J) = (G%mask2dCv(i,J) * G%mask2dCv(i+1,J) * G%mask2dCu(I,j) * G%mask2dCu(I,j-1)) | ||
| do J=Jsq-2,Jeq+1 ; do I=Isq-2,Ieq+1 |
There was a problem hiding this comment.
I suspect that these loop bounds should have symmetric halos, i.e., they should be do J=Jsq-1,Jeq+1 ; do I=Isq-1,Ieq+1. I agree with the corrections to the index offsets on the next line.
| call pass_vector(ubtav, vbtav, G%Domain) | ||
|
|
||
| do j=js-1,je+2 ; do i=is-1,ie+2 | ||
| ! Calculate the barotropic horizontal tension | ||
| do j=Jsq-2,Jeq+2 ; do i=Isq-2,Ieq+2 |
There was a problem hiding this comment.
This is tracer point loop, so I suspect that the right loop ranges might be do j=js-2,je+2 ; do i=is-2,ie+2 (at least with symmetric memory). Alternatively because they are used for subsequent calculations at the vorticity points, and we might have Isq = is or is-1, depending on whether this is symmetric or dynamic memory, this loop might also be valid as do j=Jsq-1,Jeq+2 ; do i=Isq-1,Ieq+2.
| !### Work on a wider halo to eliminate this blocking send! | ||
| call pass_var(GME_flux_h, G%Domain) | ||
| ! Update halos if needed | ||
| if (s >= 2) call pass_var(GME_flux_h, G%Domain) |
There was a problem hiding this comment.
This would be a very good candidate for using wide halos and marching inward with each iteration to avoid having to do the halo updates every iteration. (See the barotropic solver for an example of this.)
| GME_flux_q_original(:,:) = GME_flux_q(:,:) | ||
| ! apply smoothing on GME | ||
| do J = G%JscB, G%JecB | ||
| do I = G%IscB, G%IecB | ||
| do J = Jsq-1, Jeq |
There was a problem hiding this comment.
These q-point loop bounds should probably have symmetric halos around the q-point computational domain extents.
| enddo ; enddo | ||
|
|
||
| do J=js-1,Jeq ; do I=is-1,Ieq | ||
| do J=Jsq-2,Jeq+1 ; do I=Isq-2,Ieq+1 |
There was a problem hiding this comment.
These q-point loop ranges should have symmetric halos with respect to the q-point computational domain or non-symmetric halos with respect to the h-points. I.e., one of do J=Jsq-1,Jeq+1 ; do I=Isq-1,Ieq+1 or do J=js-2,je+1 ; do I=is-2,ie+1 should be right, depending on how str_xy_GME is used later.
| (MIN(htot(i,j) * I_GME_h0, 1.0)**2) | ||
| else | ||
| GME_effic_h(i,j) = 0.0 | ||
| endif | ||
| enddo ; enddo | ||
|
|
||
| do J=js-1,Jeq ; do I=is-1,Ieq | ||
| do J=Jsq-2,Jeq+1 ; do I=Isq-2,Ieq+1 |
There was a problem hiding this comment.
This is another case where there should be symmetric halos based on the q-point computational domain, or non-symmetric halos based on the tracer point loop limits - i.e., do J=Jsq-1,Jeq+1 ; do I=Isq-1,Ieq+1 or do J=js-2,je+1 ; do I=is-2,ie+1, depending on the later use of GME_effic_q.
| (0.25*((dvdx_bt(I,J)+dvdx_bt(I-1,J-1))+(dvdx_bt(I,J-1)+dvdx_bt(I-1,J))))**2 + & | ||
| (0.25*((dudy_bt(I,J)+dudy_bt(I-1,J-1))+(dudy_bt(I,J-1)+dudy_bt(I-1,J))))**2) | ||
| enddo ; enddo | ||
|
|
||
| do J=js-2,Jeq+1 ; do I=is-2,Ieq+1 | ||
| grad_vel_mag_bt_q(I,J) = boundary_mask_q(I,J) * (dvdx_bt(I,J)**2 + dudy_bt(I,J)**2 + & | ||
| do J=Jsq-2,Jeq+1 ; do I=Isq-2,Ieq+1 |
There was a problem hiding this comment.
See the other comments about the q-point loop bounds. They apply here too.
There was a problem hiding this comment.
Overall I think that these changes are definitely a big step in the right direction. The fact that the solutions are now reproducing across PE layouts is a strong indication that the loop extents are wide enough. There may, however, be some that are one point wider than they need to be, and I have noted these in the comments below. If you are able to reproduce answers, in both symmetric and non-symmetric memory mode, with slightly narrower halos, that could make the code a little more efficient. A bigger opportunity for increasing the throughput would be to use the wide-halo techniques and march the halos inward one point with each iteration (as is done in the barotropic solver) with the code in smooth_GME, thereby avoiding the need to do a halo update with each iteration. However, all of these comments are on points of efficiency, and I am confident that all of the actual non-repoducibility we had before should have been addressed. In my view we could either take this important PR as it stands, or send it back for some minor tweaks to improve performance, depending on the preference of @gustavo-marques and @sdbachman .
|
Hi Bob, I think you should take the PR as it stands and we can return to
the efficiency issues later. There are people who are waiting on a
functional version of GME to continue their research, and I don't want to
keep them waiting while we fiddle with this.
…On Wed, Feb 16, 2022 at 8:57 AM Robert Hallberg ***@***.***> wrote:
***@***.**** commented on this pull request.
Overall I think that these changes are definitely a big step in the right
direction. The fact that the solutions are now reproducing across PE
layouts is a strong indication that the loop extents are wide enough. There
may, however, be some that are one point wider than they need to be, and I
have noted these in the comments below. If you are able to reproduce
answers, in both symmetric and non-symmetric memory mode, with slightly
narrower halos, that could make the code a little more efficient. A bigger
opportunity for increasing the throughput would be to use the wide-halo
techniques and march the halos inward one point with each iteration (as is
done in the barotropic solver) with the code in smooth_GME, thereby
avoiding the need to do a halo update with each iteration. However, all of
these comments are on points of efficiency, and I am confident that all of
the actual non-repoducibility we had before should have been addressed. In
my view we could either take this important PR as it stands, or send it
back for some minor tweaks to improve performance, depending on the
preference of @gustavo-marques <https://github.com/gustavo-marques> and
@sdbachman <https://github.com/sdbachman> .
—
Reply to this email directly, view it on GitHub
<#65 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE5PGIWBETL7PJVX73DMMNDU3PCNDANCNFSM5NNF7EPA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
This commit does appear to address the issues with the previous version not reproducing across PE layout and logging parameters in a context where they are not relevant. It has passed TC testing, and it has passed the MOM6-examples pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14805 .
There are several obvious steps (noted in the comments) that could improve the computational efficiency of this code, but in the interests of making a properly working version of the code available, these improvements will be deferred to a subsequent pull request.
Given the large number of logically connected commits in this PR, and the fact that only the final version is passing the testing, I am going to do a squash-merge in this PR.
This PR does change the contents of some of the MOM_parameter_doc.all files, so MOM6-examples will also need to be updated.
Fixed the bug noted in issue #72 and excessive loop sizes noted in the unresolved comments in a recent commit, and cleaned up other aspects of MOM_hor_visc, mostly related to the code that is exercised when USE_GME=True. - Fixed the bug with the wrong arrays being used when ADD_LES_VISCOSITY=True that was noted in MOM6 issue #72. - Corrected some of the overly large loop extents that were noted in unresolved comments about MOM6 PR #65. - Only log USE_KH_BG_2D if a Laplacian viscosity is used. - Use extra calculations in the halos and marching in to avoid unnecessary halo updates in smooth_GME if there multiple smoothing passes. - Corrected the capitalization of some horizontal indices to follow the MOM6 convention for indicating the horizontal staggering position. - Collected the post_data calls for diagnostics with use_GME with the other post data calls to collocate some of the potential inter-PE synchronization points. - Fixed the indentation of some expressions that were using less than 4 points for some continuation lines. - Eliminated several unused variables, and fused some loops to allow 2-d variables to be replaced with scalars. With this PR, answers can change when ADD_LES_VISCOSITY=True and there is a Smagorinsky or Leith Laplacian viscosity and there is a nonzero background. One run-time parameter is no longer logged if LAPLACIAN=false, so there are changes to the MOM_parameter_doc files. All answers in the MOM6-examples test suite are bitwise identical.
Fixed the bug noted in issue #72 and excessive loop sizes noted in the unresolved comments in a recent commit, and cleaned up other aspects of MOM_hor_visc, mostly related to the code that is exercised when USE_GME=True. - Fixed the bug with the wrong arrays being used when ADD_LES_VISCOSITY=True that was noted in MOM6 issue #72. - Corrected some of the overly large loop extents that were noted in unresolved comments about MOM6 PR #65. - Only log USE_KH_BG_2D if a Laplacian viscosity is used. - Use extra calculations in the halos and marching in to avoid unnecessary halo updates in smooth_GME if there multiple smoothing passes. - Corrected the capitalization of some horizontal indices to follow the MOM6 convention for indicating the horizontal staggering position. - Collected the post_data calls for diagnostics with use_GME with the other post data calls to collocate some of the potential inter-PE synchronization points. - Fixed the indentation of some expressions that were using less than 4 points for some continuation lines. - Eliminated several unused variables, and fused some loops to allow 2-d variables to be replaced with scalars. With this PR, answers can change when ADD_LES_VISCOSITY=True and there is a Smagorinsky or Leith Laplacian viscosity and there is a nonzero background. One run-time parameter is no longer logged if LAPLACIAN=false, so there are changes to the MOM_parameter_doc files. All answers in the MOM6-examples test suite are bitwise identical.
In mom-ocean#1536, @Hallberg-NOAA identified issues with the GME code and
get_paramcalls related to the Leith_* options. As summarized below, the changes introduced here fix all the issues mentioned in the referred PR and also include additional code improvements:GME_NUM_SMOOTHINGS) to allow the user to control how many smoothing passes should be done (c2dc71f);FrictWork_GMEdiagnostic. The calculation now mimics the calculation ofFrictWorkand itincludes the energy diffusion term.
Tested using
MOM6-examples/ocean_only/global_ALE/zand NW2 1/2 deg. Thanks to @sdbachman for help fixing these issues.