part of cam6_3_008: Replace serial reads with infld calls. #241

patcal · 2020-10-06T06:33:51Z

This is an incremental revision to the nudging.F90 module which replaces serial reads with infld() calls so that PIO is used to read values on known model grids from an open netcdf file. This update does not address the need for in-line horizontal/vertical interpolations.

Fixes #237

brian-eaton · 2020-10-06T20:48:20Z

src/physics/cam/nudging.F90

+   call cam_pio_openfile(fileID,trim(anal_file),0)
+   call pio_seterrorhandling(fileID,PIO_BCAST_ERROR   )
+   call pio_seterrorhandling(fileID,PIO_INTERNAL_ERROR)


Why are the error handling calls back to back? Internal error is the default, and normally you'd change that to broadcast error while you do checks that may fail, and later switch back to the default. This code doesn't seem to be doing anything.

I used the code in metdata.F90 as a guide for my modifications. There were originally a couple of pio_inq_var() calls between them in the section of code I was referencing. The pio_inq calls were not relevant for my usage, but because I am not familiar with the PIO error handling functionality, I thought it best to include both of the pio_seterrorhandling() calls. Since they do nothing, they can certainly be removed.

gold2718 · 2020-10-06T21:33:22Z

Murphy ensures that whenever you select some code as a guide, you will select the worst possible example :-)
Also, the interface has changed so the best approach is to include these at the beginning of your routine:

   integer err_handling
...
       call pio_seterrorhandling(File, PIO_BCAST_ERROR, oldmethod=err_handling)

and this at the end of your routine.

    ! Back to old error handling
    call pio_seterrorhandling(File, err_handling)

This will reset the error handling back to whatever it was which is important if your routine is called from a region that has already changed the default PIO error handling method.

nusbaume

Mostly looks good to me, beyond the seterrorhandling issue found previously. However, I do have a few concerns which I would like to see addressed, if possible. Thanks!

src/physics/cam/nudging.F90

nusbaume · 2020-10-07T21:35:23Z

src/physics/cam/nudging.F90

-     endif
+   grid_id = cam_grid_id('physgrid')
+   call cam_grid_get_dim_names(grid_id,dim1name,dim2name)
+!   if(masterproc) write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name


For all of the commented out if(masterproc) write... statements here and below, I would recommend either un-commenting or deleting them.

Another technique is to define a debug private logical symbol at module scope which you can set to .true. when you want these messages.

Suggested change

! if(masterproc) write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name

if(masterproc .and. debug) then

write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name

end if

src/physics/cam/nudging.F90

gold2718

Mostly supporting previous change requests (just added a suggestion).

gold2718 · 2020-11-09T20:06:18Z

src/physics/cam/nudging.F90

-     endif
+   grid_id = cam_grid_id('physgrid')
+   call cam_grid_get_dim_names(grid_id,dim1name,dim2name)
+!   if(masterproc) write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name


Another technique is to define a debug private logical symbol at module scope which you can set to .true. when you want these messages.

Suggested change

! if(masterproc) write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name

if(masterproc .and. debug) then

write(iulog,*)'PIO: DIM1NAME=',dim1name,' DIM2NAME=',dim2name

end if

fvitt · 2021-01-05T20:11:47Z

What issue does this PR fix? Does this address scalability issues?

fvitt · 2021-01-05T20:18:30Z

src/physics/cam/nudging.F90

   call mpibcast(Nudge_File_Present, Nudge_NumObs, mpilog, 0, mpicom)
   call mpibcast(Nudge_ObsInd      , Nudge_NumObs, mpiint, 0, mpicom)


Should mpi_bcast (without ifdef SPMD) be used rather than mpibcast?

gold2718 · 2021-01-05T20:31:51Z

What issue does this PR fix? Does this address scalability issues?

#237, thanks for noticing, I updated the description.

fvitt · 2021-01-05T20:43:37Z

Are we expecting this regression test to be restored. I am hoping someone checks that it is bit-for-bit compared to cam6_3_006.

FAIL ERP_Ln9.ne30_ne30_mg17.FCnudged.cheyenne_intel.cam-outfrq9s RUN time=39
Nudging code is using global interfaces not supported for weak scaling.
This error should be fixed with PR #241

gold2718 · 2021-01-05T20:48:08Z

Are we expecting this regression test to be restored

Yes, I will make sure of that.

nusbaume

Looks good to me! Just one typo in all the new endrun calls.

nusbaume · 2021-01-05T20:56:24Z

src/physics/cam/nudging.F90

+   if(VARflag) then
+     Nobs_U(:,:,begchunk:endchunk,Nudge_ObsInd(1)) = Tmp3D(:,:,begchunk:endchunk)
+   else
+     call endrun('Varibale "U" is missing in '//trim(anal_file))


Misspelled variable in the endrun calls here and below.

gold2718 · 2021-01-07T04:07:50Z

From around line 772:

call endrun('NUDGING IS CURRENTLY ONLY CONFIGURED FOR CAM-SE, FV, or EUL')

Is this still true? Should it work with MPAS and/or FV3?

patcal · 2021-01-07T15:38:08Z

The module originally needed different interfaces to read in the data for
each dycore. FV3 and MPAS are new so there is no interface set up for them,
but if you notice, with the change to infld calls all of the nudging_update_analyses_*()
subroutines are now identical. So I think that adding dycore_is() cases for the 2
new dycores to the if-elseif block and having them call nudging_update_analyses_se()
will allow for FV3 and MPAS. I left the degeneracy for the time being in case any
dycore-dependent differences showed up during testing and needed to be added
back in. Rather than doing it after the module is fully tested, an alternative would be
to remove the if-block and the redundant routines now.

Fix remaining PR issues in nudging.F90 Update ChangeLog

gold2718 · 2021-01-10T22:28:14Z

The changes from this PR were merged to cam_development as part of cam6_3_008 (#310).

For Issue ESCOMP#237: Replace serial reads with infld calls.

2e81271

gold2718 requested review from cacraigucar, gold2718 and nusbaume October 6, 2020 16:27

gold2718 self-assigned this Oct 6, 2020

gold2718 added the enhancement New feature or request label Oct 6, 2020

gold2718 requested review from brian-eaton and fvitt October 6, 2020 16:28

gold2718 added this to the CESM2.3 milestone Oct 6, 2020

brian-eaton reviewed Oct 6, 2020

View reviewed changes

nusbaume requested changes Oct 7, 2020

View reviewed changes

gold2718 requested changes Nov 9, 2020

View reviewed changes

gold2718 mentioned this pull request Dec 31, 2020

cam6_3_007: Initial implementation of weak scaling physics infrastructure #287

Merged

Added requested modifications

da08a01

patcal requested review from gold2718 and nusbaume January 4, 2021 22:16

fvitt reviewed Jan 5, 2021

View reviewed changes

nusbaume approved these changes Jan 5, 2021

View reviewed changes

Revised nudging.F90 for typos / dycore dependence

b8797b9

patcal requested review from fvitt and nusbaume January 7, 2021 16:29

nusbaume approved these changes Jan 7, 2021

View reviewed changes

gold2718 pushed a commit to gold2718/CAM that referenced this pull request Jan 8, 2021

Import nudging.F90 from patcal's PR ESCOMP#241

a7c9031

Fix remaining PR issues in nudging.F90 Update ChangeLog

gold2718 mentioned this pull request Jan 8, 2021

cam6_3_008: Fixes for PIO2 plus nudging I/O update #310

Merged

gold2718 closed this Jan 10, 2021

cacraigucar changed the title ~~Replace serial reads with infld calls.~~ part of cam6_3_008: Replace serial reads with infld calls. Jan 11, 2021

		call mpibcast(Nudge_File_Present, Nudge_NumObs, mpilog, 0, mpicom)
		call mpibcast(Nudge_ObsInd , Nudge_NumObs, mpiint, 0, mpicom)

part of cam6_3_008: Replace serial reads with infld calls. #241

part of cam6_3_008: Replace serial reads with infld calls. #241

Uh oh!

Conversation

patcal commented Oct 6, 2020 • edited by gold2718 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-eaton Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

patcal Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

gold2718 commented Oct 6, 2020

Uh oh!

nusbaume left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nusbaume Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

gold2718 Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

gold2718 Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

fvitt commented Jan 5, 2021

Uh oh!

fvitt Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

gold2718 commented Jan 5, 2021

Uh oh!

fvitt commented Jan 5, 2021

Uh oh!

gold2718 commented Jan 5, 2021

Uh oh!

nusbaume left a comment

Choose a reason for hiding this comment

Uh oh!

nusbaume Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

gold2718 commented Jan 7, 2021

Uh oh!

patcal commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gold2718 commented Jan 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

patcal commented Oct 6, 2020 •

edited by gold2718

Loading

patcal commented Jan 7, 2021 •

edited

Loading