User/lmh/fv gfs #97

lharris4 · 2019-07-17T21:02:04Z

This includes Zhi's updates for the new nesting capabilities. I don't know if Zhi pushed his changes to the official FMS repo or even if he has authorized such a thing.

Lucas

…arse_update branch

…r the infamous 'compute_qs' crash

…to lmh_ncep_regional

…c-output-fails-to-write-history-file' into user/lmh/fvGFS

…to user/lmh/fvGFS

wrongkindofdoctor · 2019-07-18T15:11:29Z

None of Zhi's changes to the mpp_domains files have been pushed to the GFDL master because Ray had problems compiling with them. Zhi made the changes on a branch that was out of sync with the main I/O development branch, so FMS will test the domains once we have worked through the I/O.

lharris4 · 2019-07-18T15:20:56Z

Hi, Jess. Thanks. There are updates to the dynamical core that are necessary for Zhi's changes to work. I am preparing a "sanitized" version of the core for transmission outside of GFDL that will include the changes in FV3 to support the FMS changes but that will not include codes that should not be made public yet. Lucas

…

On Thu, Jul 18, 2019 at 11:11 AM Jess ***@***.***> wrote: None of Zhi's changes to the mpp_domains files have been pushed to the GFDL master because Ray had problems compiling with them. Zhi made the changes on a branch that was out of sync with the main I/O development branch, so FMS will test the domains once we have worked through the I/O. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#97?email_source=notifications&email_token=AMUQRVGZW7T36HFAOOVSPDTQACB2FA5CNFSM4IEUPUO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2IZRNY#issuecomment-512858295>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMUQRVBMBBF2CYKVHOESKVTQACB2FANCNFSM4IEUPUOQ> .

underwoo

There are some updates I would like to see added, mainly documentation and comments to help explain why choices were made. I'm sure Zhi made many of the changes, so getting the information may be difficult. We should at least make an effort to address some of these.

I would also like to see a result (regression-like test) of a run using these updates.

Lastly, two of the files had extensive updates. I, unfortunately, am not the correct person to review those changes. Please get another to at least review the changes in mpp/include/mpp_update_nest_domains.h and mpp/test_mpp_domains.F90. The test_mpp_domains.F90 review needs to ensure that the tests do test the new code.

underwoo · 2019-08-07T15:06:48Z

mpp/mpp_domains.F90

  type nest_domain_type
+     character(len=NAME_LENGTH)     :: name


Where is the documentation for the nest_domain_type and nest_level_type? Documentation of the derived types is something we should always have.

underwoo · 2019-08-07T15:17:11Z

mpp/include/mpp_domains_define.inc

@@ -1347,6 +1349,9 @@ end subroutine check_message_size
       end if
    end do

+    allocate(domain%tile_id_all(num_tile))


This is a comment about code not touched: The tile_id_local allocatable array is not deallocated in this routine. While most compilers will, possibly, automatically deallocate it is much safer to add a deallocate in the routine. I'm not going to have this hold up the merge, but it is something that should be addressed.

underwoo · 2019-08-07T15:18:17Z

mpp/include/mpp_domains_misc.inc

@@ -512,7 +512,7 @@ end subroutine init_nonblock_type
      logical :: native         !true if I'm on the pelist of this domain
      integer :: listsize, listpos
      integer :: n
-      integer, dimension(11) :: msg, info         !pe and compute domain of each item in list
+      integer, dimension(12) :: msg, info         !pe and compute domain of each item in list


I do not like this 12 (previously 11) to be coded like this. It does not allow future developers to know why the dimension of these arrays is specifically 12.

underwoo · 2019-08-07T15:19:30Z

mpp/include/mpp_domains_misc.inc

@@ -571,12 +571,13 @@ end subroutine init_nonblock_type
      else
         info(11) = 0
      endif
+      info(12) = domain%ntiles


I am concerned if the info(11) = 0 on line 572 should be info(12) = 0. Without knowing why info has 12 elements, I am unable to determine the reason.

underwoo · 2019-08-07T15:19:44Z

mpp/include/mpp_domains_misc.inc

 !broadcast your info across current pelist and unpack if needed
      listpos = 0
      do n = 0,mpp_npes()-1
         msg = info
         if( mpp_pe().EQ.pes(n) .AND. debug )write( errunit,* )'PE ', mpp_pe(), 'broadcasting msg ', msg
-         call mpp_broadcast( msg, 11, pes(n) )
+         call mpp_broadcast( msg, 12, pes(n) )


Here is that 12 again.

underwoo · 2019-08-07T15:22:30Z

mpp/include/mpp_domains_misc.inc

+      logical :: native         !true if I'm on the pelist of this domain
+      integer :: listsize, listpos, nestsize(size(tile_nest(:)))
+      integer :: n, tile, ind, num_nest
+      integer, dimension(15) :: msg, info         !pe and compute domain of each item in list


And now we have 15 elements.

underwoo · 2019-08-07T15:24:32Z

mpp/mpp_domains.F90

@@ -319,6 +325,7 @@ module mpp_domains_mod
  type domain1D_spec
     private
     type(domain_axis_spec) :: compute
+     type(domain_axis_spec) :: global


derived type documentation?

underwoo · 2019-08-07T15:25:38Z

sat_vapor_pres/sat_vapor_pres.F90

@@ -535,7 +535,7 @@ module sat_vapor_pres_mod

 ! The default values below preserve the behavior of omsk and earlier revisions.
 logical :: show_bad_value_count_by_slice=.true.
- logical :: show_all_bad_values=.false.
+ logical :: show_all_bad_values=.true. ! Changed LMH 22may2018


A change in default behavior should be included in the release notes.

bensonr · 2019-08-07T19:49:17Z

sat_vapor_pres/sat_vapor_pres.F90

@@ -535,7 +535,7 @@ module sat_vapor_pres_mod

 ! The default values below preserve the behavior of omsk and earlier revisions.
 logical :: show_bad_value_count_by_slice=.true.
- logical :: show_all_bad_values=.false.
+ logical :: show_all_bad_values=.true. ! Changed LMH 22may2018


Is there universal acceptance for changing the default behavior of a namelist controlled variable?

bensonr

In mpp_do_update_nest.h we define nest_domain to be type nest_level_type at lines 23 and 264, but at line 860 we define nest as nest_level_type and have nest_domain as nest_domain_type. It would be beneficial to have consistent naming within this file to keep confusion to a minimum. I can make incorporate this branch into my fork and make the requested changes.

In mpp_define_nest_domains.inc there are times where nest_level is an integer and nest_domains is nest_level_type. This can be confusing for future developers.

In mpp_update_nest_domains.h nest_level is used as an integer and may be confused with nest_level_type for future developers.

bensonr and others added 13 commits January 24, 2018 13:51

hand merge of user/z1l/nest_coarse_update into user/rab/fv3_merge

7fe8561

added ifdef/endif pairs for use_NETCDF as suggested by Zhi

a72dfc8

added refine_ratio to the test_mpp_domains_nml as per Zhi

614ae66

updated mpp/include/mpp_define_nest_domains.inc from user/z1l/nest_co…

200ba9b

…arse_update branch

Made default the options that enable more specific error reporting fo…

9b4195b

…r the infamous 'compute_qs' crash

Merge remote-tracking branch 'origin/user/z1l/nest_coarse_update2' in…

8a76499

…to lmh_ncep_regional

Revert to remove unfinished code

08bb36d

extend mpp_domains to support multiple level nesting (telescoping)

20ca63c

fix compilation issue due to merge

9c2c68e

made extra global_axis_size variable, fixed conditional

48a5577

Merge remote-tracking branch 'origin/53-non-square-regional-diagnosti…

2a36364

…c-output-fails-to-write-history-file' into user/lmh/fvGFS

Merge remote-tracking branch 'origin/user/z1l/nest_coarse_update3' in…

f51aeeb

…to user/lmh/fvGFS

Change to support different refinement ratios on different grids

7383551

wrongkindofdoctor self-assigned this Aug 6, 2019

wrongkindofdoctor requested review from underwoo and removed request for underwoo August 6, 2019 15:26

underwoo requested a review from bensonr August 7, 2019 14:55

underwoo reviewed Aug 7, 2019

View reviewed changes

bensonr reviewed Aug 7, 2019

View reviewed changes

bensonr reviewed Aug 8, 2019

View reviewed changes

underwoo added this to the 2020.01 milestone Nov 27, 2019

underwoo added the development label Nov 27, 2019

underwoo closed this Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User/lmh/fv gfs #97

User/lmh/fv gfs #97

lharris4 commented Jul 17, 2019

wrongkindofdoctor commented Jul 18, 2019

lharris4 commented Jul 18, 2019 via email

underwoo left a comment •

edited

Loading

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

underwoo Aug 7, 2019

bensonr Aug 7, 2019

bensonr left a comment

User/lmh/fv gfs #97

User/lmh/fv gfs #97

Conversation

lharris4 commented Jul 17, 2019

wrongkindofdoctor commented Jul 18, 2019

lharris4 commented Jul 18, 2019 via email

underwoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensonr left a comment

Choose a reason for hiding this comment

underwoo left a comment •

edited

Loading