cam6_3_008: Fixes for PIO2 plus nudging I/O update#310
cam6_3_008: Fixes for PIO2 plus nudging I/O update#310gold2718 merged 4 commits intoESCOMP:cam_developmentfrom
Conversation
Fix remaining PR issues in nudging.F90 Update ChangeLog
|
@patcal, For some reason, I cannot request you as a reviewer. Please have a look at nudging.F90 which I hope has all of your changes plus some cleanup from me. |
|
|
||
| end function cam_pio_fileexists | ||
|
|
||
| integer function cam_pio_set_fill(File, fillmode, old_mode) result(ierr) |
There was a problem hiding this comment.
It looks like the return value of ierr is not set if PIO2 is not defined.
There was a problem hiding this comment.
Nice catch, thanks!
Look at the change to dyn_grid.F90 starting at line 1116. When FV is in a 2-D decomposition, there is more than one task containing a particular latitude band (zonal mean value) and I was trying to write to that slot on the file from each of those tasks. The change makes sure that only one of those tasks will try to write. This bug does not occur in a 1-D decomposition and PIO1 was perfectly happy to do the multiple writes from a 2-D decomposition but it seems like a really bad idea and PIO2 agrees :-) |
nusbaume
left a comment
nusbaume
left a comment
There was a problem hiding this comment.
Generally looks good, but I do have a few questions/concerns.
| Purpose of changes (include the issue number and title text for each | ||
| relevant GitHub issue): | ||
| #248: scam tests don't work with pio2 | ||
| #263: attempt to initialize variable prior to calling intent(out) subroutine |
| restartvars(i)%ifill) | ||
| else if(restartvars(i)%type == PIO_REAL) then | ||
| ierr = pio_put_att(File, restartvars(i)%vdesc, "_FillValue", & | ||
| restartvars(i)%rfill) |
There was a problem hiding this comment.
It doesn't look like rfill is initialized above. Is that ok?
There was a problem hiding this comment.
My understanding is that this would be initialized if any variable were actually using that kind. As an example, dfill for fillvalue is initialized on line 1144 so it will be initialized when written out on line 1304.
@jedwards4b, do I understand that correctly?
There was a problem hiding this comment.
no, I think that it should be - good catch.
There was a problem hiding this comment.
Sorry, you are correct. This variable will not ever be written as PIO_REAL so this branch is never used.
| restartvars(rvindex)%dims(1) = maxnflds_dim_ind | ||
| restartvars(rvindex)%dims(2) = ptapes_dim_ind | ||
| restartvars(rvindex)%fillset = .true. | ||
| restartvars(rvindex)%ifill = 0 |
There was a problem hiding this comment.
I am probably misunderstanding the code, but is zero the best choice for a fill value? It seems like there could be situations where the variable really would have a value of zero. Should we just use the default PIO fill values for each type (e.g. PIO_FILL_INT) instead?
There was a problem hiding this comment.
@nusbaume previously the code used an implied fill value of 0. This is just continuing the previous behavior with pio2, the reason this needs to be done for pio2 and not pio1 is that pio1 wrote values for the entire array even out of bounds points but pio2 only writes the values it needs to write and sets the rest to _FillValue. I agree that 0 is not the best fill value to consider, but any other value would require more changes in cam.
There was a problem hiding this comment.
In this case, it is a good fill value because it is an invalid value for that item (decomp_type). The same goes for the other integer restart values.
nusbaume
left a comment
nusbaume
left a comment
There was a problem hiding this comment.
@jedwards4b @gold2718 Thanks for the explanations! I am happy with this PR now.
gold2718
changed the title
6 participants
Fixes to allow CAM to operate with PIO2
Unread buffer locations properly initialized after read for SE dycore.
Fixes to nudging code to work with new weak-scaling infrastructure
Fixes #237
Fixes #248
Fixes #263
Fixes #282
Closes #241
Closes #247