initialize two arrays and fix fortran coding error plus PRs #285 and #276 #280
Conversation
SamuelTrahanNOAA
changed the title
| @@ -141,6 +142,7 @@ subroutine fv_dyn_restart_register (Atm) | |||
| nvar3d_tracers = ntprog+ntdiag | |||
| tracers_zsize = size(Atm%q,3) | |||
| allocate (tracers_var3(nx,ny,tracers_zsize,nvar3d_tracers), tracers_var3_names(nvar3d_tracers)) | |||
| tracers_var3 = 0.0 | |||
There was a problem hiding this comment.
Best practice in multi-threaded simulations is to loop over elements to ensure each cell is allocated on the correct thread, although modern compilers might get around this.
There was a problem hiding this comment.
Normally you would be right, but not here. The correct thread is the master thread.
The rest of fv_ufs_restart_io.F90 does not use OpenMP directives. Hence, tracers_var3 is only ever used by the master thread. Using OpenMP to initialize tracers_var3 would distribute it across multiple CPUs. If some threads are in different NUMA nodes than the master thread, then this will slow down the program.
Adding OpenMP directives throughout fv_ufs_restart_io.F90 is outside the scope of this PR. Also, there may be good reasons why the restart code is single-threaded. I can't think of any, but somebody probably implemented it this way for a reason.
|
Hi, Zach. I hadn't realized I was a reviewer. I have approved the PR and it
should be able to move forward soon.
Thanks
Lucas
…On Thu, Sep 21, 2023 at 3:45 PM zach1221 ***@***.***> wrote:
Hello, @lharris4 <https://github.com/lharris4> @bensonr
<https://github.com/bensonr> are you able to continue reviewing this PR?
—
Reply to this email directly, view it on GitHub
<#280 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVGGEXFYLS5RGO4LSADX3SKNXANCNFSM6AAAAAA4Q56CFU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…ables to .false. when files are closed (NOAA-GFDL#284)
|
BTW @bensonr is on leave today ; he should be back on Monday. |
|
I've merged #283 into this PR since that one is expected to go in first. This is just to speed up the UFS test process. In the unlikely event that #283 is rejected, I'll back out those changes. EDIT: The testing of #283, at the ufs-weather-model level, as ufs-community/ufs-weather-model#1903, is almost done. |
|
Thanks for taking the lead on this. These changes should be fine.
Lucas
…On Mon, Sep 25, 2023 at 1:48 PM Samuel Trahan (NOAA contractor) < ***@***.***> wrote:
I've merged #283
<#283> into this
PR since that one is expected to go in first. This is just to speed up the
UFS test process.
In the unlikely event that #283
<#283> is
rejected, I'll back out those changes.
—
Reply to this email directly, view it on GitHub
<#280 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVEPC2HB6HCU7R5U4JLX4G7VPANCNFSM6AAAAAA4Q56CFU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
As you've included this here, should we have @DusanJovic-NOAA close the #283? |
No. That PR should be merged now. |
|
@SamuelTrahanNOAA I have just merged PR 283 into the dev/emc branch, but this PR is still showing the changes from PR 283 in the list of changes. Could you please merge in dev/emc branch to this branch? I believe that GitHub is confusing your merge of Dusan's changes in commit 81f254d with the actual PR 283 dev/emc merge commit. |
I have merged dev/emc to this branch. Please wait to merge my PR until the UFS code managers finish testing it at the ufs-weather-model level. |
…tring-length-mismatch
|
This PR now includes #285 and #276 by request of @laurenchilutti. I've tested #285 on six platforms. I'll begin to test #276 shortly. |
SamuelTrahanNOAA
changed the title
|
Could @junwang-noaa and @laurenchilutti please confirm that your PRs were merged into this one correctly? |
|
@laurenchilutti As @SamuelTrahanNOAA 's mini-test went ok through ufs-community/ufs-weather-model#1893, I don't see any major issue to merge this pr. Can you go ahead to merge in this pr by the end of today? |
Description
#279: fortran coding error: string length mismatch
Wherein a string length mismatch causes an abort. The code does not follow the fortran standard on this line:
because the two strings in the array have different lengths. See #279 for details.
#281: uninitialized arrays
The srf_wnd_var2 and tracers_var3 arrays are not initialized after allocation in fv_ufs_restart_io.F90. This can cause the ufs-weather-model write component's quilting restart to fail. When ESMF copies data from compute ranks to write ranks, it'll encounter NaN values if the uninitialized array happen to contain any.
How Has This Been Tested?
ufs-weather-model regression tests.
Specifically, the gnu version of the
conus13km_qr_debugregression test.The regression test can be found in this branch. To see the failure, disable this bug fix.
ufs-community/ufs-weather-model#1893
Checklist:
Please check all whether they apply or not