-
Notifications
You must be signed in to change notification settings - Fork 119
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
Code changes related to EMC FMS mixedmode #163
Conversation
* update tools/fv_nudge.F90 * modify the following files to use 'constantsR4_mod' module: driver/fvGFS/atmosphere.F90 driver/fvGFS/fv_nggps_diag.F90 model/fv_dynamics.F90 model/fv_nesting.F90 model/fv_update_phys.F90 model/nh_core.F90 model/nh_utils.F90 tools/coarse_grained_diagnostics.F90 tools/external_ic.F90 tools/fv_diagnostics.F90 tools/fv_nudge.F90 tools/fv_restart.F90 tools/test_cases.F90 * update external_ic.F90 and fv_nudge.F90
* revise external_ic.F90 and fv_nudge.F90 to use allocatable arrays
tools/fv_nudge.F90
Outdated
@@ -3545,16 +3545,17 @@ subroutine pmaxmin( qname, a, imax, jmax, fac ) | |||
class(*) a(imax,jmax) | |||
class(*) fac ! multiplication factor | |||
|
|||
real(r4_kind) qmin(jmax), qmax(jmax) | |||
real(r4_kind), dimension(:), allocatable :: qmin, qmax |
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.
Why do these need to be allocatable arrays when we already know their length?
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.
allocatable
is used to save memory: Depending on whether it is dealing with single or double data (from FMS), either (qmin
, qmax
) OR (qmin8
, qmax8
) is required, but not both; the selection and allocation is done inside the select type
construct below.
tools/fv_nudge.F90
Outdated
@@ -3545,16 +3545,17 @@ subroutine pmaxmin( qname, a, imax, jmax, fac ) | |||
class(*) a(imax,jmax) |
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.
What is "class(*)"? Is this new Fortran functionality, and if so what version?
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.
It is F2003. These changes are part of the efforts to allow FMS to handle both 32- and 64-bit interfaces.
These are 1D arrays. We're talking about a pretty negligible amount of
memory! But this allocate-deallocate could lead to efficiency problems,
especially vectorization, and makes the code more complicated for little
benefit.
…On Wed, Dec 8, 2021 at 9:25 AM Minsuk Ji ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tools/fv_nudge.F90
<#163 (comment)>
:
> @@ -3545,16 +3545,17 @@ subroutine pmaxmin( qname, a, imax, jmax, fac )
class(*) a(imax,jmax)
class(*) fac ! multiplication factor
- real(r4_kind) qmin(jmax), qmax(jmax)
+ real(r4_kind), dimension(:), allocatable :: qmin, qmax
allocatable is used to save memory: Depending on whether it is dealing
with single or double data (from FMS), either (qmin, qmax) OR (qmin8,
qmax8) is required, but not both; the selection and allocation is done
inside the select type construct below.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#163 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVCFNVI4FTAKMKIP6IDUP5TGXANCNFSM5HY3IQ7Q>
.
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>.
|
@laurenchilutti We are trying to compile the mixed-mode FMS and MOM6 with r8 (double precision) and GFDL_atmos_cubed_sphere with r4 (single-precision) for a coupled application. When FMS code is compiled with r8, sst_ncep is r8 in FMS/amip_interp/amip_interp.F90 (about line 113). In GFDL_atmos_cubed_sphere, sst_ncep is from FMS's amip_interp_mod since NO_GFDL_SHARED is not defined for Fortran code preprocessing in the coupled application. Subroutine pmaxmin is used near line 1653 in tools/external_ic.F90: where sst_ncep is r8 type, so subroutine pmaxmin( qname, a, im, jm, fac ) needs to be updated to support double-precision variable “a” when external_ic.F90 is compiled with r4. Similar updates are made to subroutine pmaxmin in tools/fv_nudge.F90. Another way to update pmaxmin is to use function overloading instead of using unlimited polymorphic data structure. |
Since this is purely a diagnostic, I'd suggest simply casting sst_ncep to
the default compiled precision for FV3 and be done with it.
… Message ID: ***@***.***
.com>
|
@bensonr @laurenchilutti The updated FMS still supports 32bit and 64bit builds. Currently the sst_ncep is defined as real in FMS/amip_interp/amip_interp.F90. In the case of mixed mode coupled runs, sst_ncep is defined as real(8) in fms library as fms is compiled with r8 because of MOM6, so pmaxmin has no issue here. However since in the mixed mode runs, dycore is running with real(4), that means the pmaxmin also needs to handle input argument with real(4) data type from dycore. That is why we have to update pmaxmin. Hope this makes sense. Thanks |
The reason for making the change is well understood. An easy fix is to
ensure sst_ncep has the compiled type when using it as an argument to
pmaxmin by simply calling it like:
call pmaxmin( 'SST_ncep_fms', real(sst_ncep), i_sst, j_sst, 1.)
I'd prefer this change to making something inside the dycore polymorphic.
|
@bensonr I want to confirm with you: 1) the pmaxmin change is required because of mixed mode runs with dycore (32bit) and mom6(64bit). Dycore has other pmaxmin calls besides the one with sst_ncep. Actually sst_ncepis working without the changes in the PR. If we have following statement: Maybe I don't get your approach, I feel this is not a simple solution. |
sst_ncep is legacy code and I don't believe anyone outside of GFDL is
actively using it. I don't think this routine warrants the amount of time
being spent on it.
…On Fri, Feb 4, 2022 at 12:31 PM Jun Wang ***@***.***> wrote:
@bensonr <https://github.com/bensonr> I want to confirm with you: 1) the
pmaxmin change is required because of mixed mode runs with dycore (32bit)
and mom6(64bit). Dycore has other pmaxmin calls besides the one with
sst_ncep. Actually sst_ncepis working without the changes in the PR. If we
have following statement:
call pmaxmin( 'SST_ncep_fms', real(sst_ncep), i_sst, j_sst, 1.)
won't work without the pmaxmin updates showed in the PR because pmaxmin is
taking real(8) argument and the dycore fields are in 32BIT.
2) sst_ncep is a module variable in FMS, not in dycore. We can set
sst_ncep with class(*) in FMS as you suggested, but then more dycore
updates are required in (model/fv_grid_utils.F90, tools/external_ic.F90,
tools/fv_nudge.F90) beside the pmaxmin change in this PR.
Maybe I don't get your approach, I feel this is not a simple solution.
—
Reply to this email directly, view it on GitHub
<#163 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVGRJ2RTFTAE6Y2WDQ3UZQENJANCNFSM5HY3IQ7Q>
.
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 commented.Message ID:
***@***.***>
|
@bensonr I just found out that I was in wrong directory when I read the code changes, sorry I misunderstood the changes. The pmaxmin subroutine is showed up twice in dycore, it is not defined in FMS. So it makes sense just to have "call pmaxmin( 'SST_ncep_fms', real(sst_ncep), i_sst, j_sst, 1.)" without making changes in pmaxmin in this PR. Sorry. |
Let us know when he changes are reverted.
|
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 resolving my comment.
* Add get_nth_domain_info subroutine * Deallocate local arrays in fv_dyn_bundle_setup * adding back a line that was mistakenly deleted in a previous commit (NOAA-GFDL#166) * Do not print debug messages to stderr * fix for 4diau with iau_filter_increments=T (NOAA-GFDL#167) * fix for 4diau with iau_filter_increments=T * fix time interval for 3DIAU * fix typo in comment * fix bug found in review by @MingjingTong-NOAA * change tnext to integer variable itnext Co-authored-by: jswhit2 <Jeffrey.S.Whitaker@noaa.gov> * Use mpp_error instead of write statements in model/fv_regional_bc.F90 * Attempt at integrating fixes on top of dev/emc branch. (NOAA-GFDL#173) Co-authored-by: Dusan Jovic <dusan.jovic@noaa.gov> Co-authored-by: laurenchilutti <60401591+laurenchilutti@users.noreply.github.com> Co-authored-by: Jeff Whitaker <jswhit@fastmail.fm> Co-authored-by: jswhit2 <Jeffrey.S.Whitaker@noaa.gov> Co-authored-by: MatthewPyle-NOAA <48285220+MatthewPyle-NOAA@users.noreply.github.com>
* Add get_nth_domain_info subroutine * Deallocate local arrays in fv_dyn_bundle_setup * adding back a line that was mistakenly deleted in a previous commit (NOAA-GFDL#166) * Do not print debug messages to stderr * fix for 4diau with iau_filter_increments=T (NOAA-GFDL#167) * fix for 4diau with iau_filter_increments=T * fix time interval for 3DIAU * fix typo in comment * fix bug found in review by @MingjingTong-NOAA * change tnext to integer variable itnext Co-authored-by: jswhit2 <Jeffrey.S.Whitaker@noaa.gov> * Use mpp_error instead of write statements in model/fv_regional_bc.F90 * Attempt at integrating fixes on top of dev/emc branch. (NOAA-GFDL#173) * Support for cloud microphysics hail species (NOAA-GFDL#171) * Incorporated Tim Supinie's updates to handle a hail category (nwat=7) * Added print for hail max/min (non-debug section) * Add hallwat to !OMP shared variables in main loop Addresses compile failure when OMP is turned on (e.g., on Hera) * Update to states as used in SFE2021 (Removed SFE code for now) * Cleanup after rebase * Redo removal of alt. namelist read * Removed tools/module_diag_hailcast.F90 * Fixes from Jili Dong * Add hailwat to tools/external_ic.F90 * Set logic in fv_regional_bc.F90 to match current code; Change logic in external_ic.F90 to check value of nwat * Recommended changes * Check actual index (hailwat) instead of assuming a positive value based on 'nwat' value * Split nwat=7 bits into separate loops (see if it affects nwat=6) Co-authored-by: Larissa Reames <52886575+LarissaReames-NOAA@users.noreply.github.com> Co-authored-by: LarissaReames-NOAA <larissa.reames@noaa.gov> Co-authored-by: Dusan Jovic <dusan.jovic@noaa.gov> Co-authored-by: laurenchilutti <60401591+laurenchilutti@users.noreply.github.com> Co-authored-by: Jeff Whitaker <jswhit@fastmail.fm> Co-authored-by: jswhit2 <Jeffrey.S.Whitaker@noaa.gov> Co-authored-by: MatthewPyle-NOAA <48285220+MatthewPyle-NOAA@users.noreply.github.com> Co-authored-by: Ted Mansell <37668594+MicroTed@users.noreply.github.com> Co-authored-by: Larissa Reames <52886575+LarissaReames-NOAA@users.noreply.github.com> Co-authored-by: LarissaReames-NOAA <larissa.reames@noaa.gov>
@MinsukJi-NOAA Could you please resolve the merge conflicts on this PR? Thanks! |
@junwang-noaa @laurenchilutti This branch has been tested with FMS 2022.03 in UFS weather model on Gaea. The current UFS baseline can be reproduced. |
@binli2337 Thank you for the testing! I will follow up with nceplibs team on fms installation. After it is installed on all the platforms, please create a PR and work with Jong/Brian on code commit. |
@junwang-noaa @binli2337 Do you use spack for the FMS library? |
Not yet, we are updating the library from hpc-stack to spark, it might come in after the FMS mixed mode updates. |
@junwang-noaa ok @rem1776 is working on updating the spack configuration file for FMS with the release for the mixed-mode updates. |
@thomas-robinson That's great, thanks for letting us know. Will check with @rem1776 when using spark for fms 2022.03 in UFS. |
@thomas-robinson would you mind tagging us when @rem1776 sends those updates to the spack repo? This way we can make sure to get it into our fork quickly and roll it out on the various HPCs we support. |
Here's the spack PR to add the latest FMS version, just put it in: |
@thomas-robinson suggest opening this as a discussion in the FMS repo. |
As noted in the closed issue (NOAA-GFDL/FMS#1014) the FMS 2022.03 updates have been merged into spack. |
* fixes io performance issues by making everyone a reader when io_layout=1,1 adds capability to use FMS feature to ignore data integrity checksums in restarts * rename enforce_rst_cksum to ignore_rst_cksum and change the default value for compatibility * updated UFS/GFS atmosphere.F90 driver as per @BinLiu-NOAA and @junwang-noaa * Regional decomposition test fix (when nrows_blend > 0) (NOAA-GFDL#194) * Add missing instance for hailwat * Regional bc blend changes to extend into interior halos and overlap on corners. Still not working for u and v. * atmosphere.F90 : add hailwat to check dyn_core.F90 : Fix from Jun Wang to correct sync of u,v fv_regional_bc.F90 : add check for nrows_blend > tile size; fix error when nrows_blend=1 * Explanatory comment added * Removed commented code * Clean old code * In fv_fill.F90, use kind_phys for kind_phys instead of hard-coding 8 byte reals. (NOAA-GFDL#193) * Expose remap_scalar and remap_dwinds to fv3-jedi (NOAA-GFDL#199) * changed interface to public * added public * removed source * mods for jedi build * Transfer changes from PR NOAA-GFDL#202 to NOAA-GFDL#199 Made small changes from PR NOAA-GFDL#202 manually. * returned ignore checksum * fixed ignore checksum * Fix several bugs in fv_regional_bc.F90 relating to uninitialized or incorrectly initialized memory. (NOAA-GFDL#219) * fixes and workarounds for uninitialized memory in fv_regional_bc * remove workarounds and fix remaining known bugs in ps_reg * a few more surface pressure bug fixes; now the test case runs in debug mode * workarounds and bug fixes from gnu compiler testing * remove -9999999 commented-out code * quiet the NaNs passed to Atmp%ps * simplify comments and explain snan * use i-1 & j-1 for two-point averages, when available * Replace many changes with PR NOAA-GFDL#220 Co-authored-by: Rusty.Benson <rusty.benson@noaa.gov> Co-authored-by: Ted Mansell <37668594+MicroTed@users.noreply.github.com> Co-authored-by: Rusty Benson <6594772+bensonr@users.noreply.github.com> Co-authored-by: Samuel Trahan (NOAA contractor) <39415369+SamuelTrahanNOAA@users.noreply.github.com> Co-authored-by: Mark Potts <33099090+mark-a-potts@users.noreply.github.com>
@junwang-noaa Could you redo the merge commit you just did to not be squashed? We want to ensure the commit history is clean. |
@binli2337 would you please redo the merge? Thanks |
@junwang-noaa I think all you would need to do is click the "revert" button next to the merge commit here NOAA-EMC#76 and then perform the merge again but ensuring that it is not squashed |
Update to dev/emc 20221017
@laurenchilutti The code is merged, would you please check? Thanks |
@binli2337 The fms 2022.04 is installed on cactus. You can load it by: prepend_path("MODULEPATH", "/apps/ops/para/libs/modulefiles/mpi/intel/19.1.3.304/cray-mpich/8.1.9") fms_ver=os.getenv("fms_ver") or "2022.04" Please see /lfs/h2/emc/nems/noscrub/jun.wang/ufs-wm/20221128/fms/ufs-weather-model/modulefiles/ufs_wcoss2.intel.lua |
Update fms_mixedmode branch
On ufs-wm side, the regression tests is done for pr 1514 . We can start merging in this pr. |
@laurenchilutti @bensonr can you merge in this pr? |
Description
This PR includes code changes by @binli2337 related to EMC FMS mixedmode (NOAA-GFDL/FMS#857). These changes are necessary for the case when Dycore is built/run in 32 bit and FMS library is built in 64 bit because some FMS constants are used as dummy arguments of subroutines found in Dycore.
How Has This Been Tested?
Code changes have been tested with the latest UFS weather model (
ad73e8a
) regresstion tests.Checklist:
Please check all whether they apply or not