Skip to content
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.

Sign up for GitHub

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

Properly import coupling fields when running with separate run phases #406

Merged
merged 29 commits into from
Oct 5, 2021
Merged

Properly import coupling fields when running with separate run phases #406

Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f78aa42
Reset to zero coupling arrays for accumulated snow,
rmontuoro Dec 3, 2019
75654ac
Properly set kind type of literal constants
rmontuoro Dec 3, 2019
2870f5e
Initialize to zero canopy resistance output variable
rmontuoro Dec 3, 2019
37fb245
Re-implement radiation diagnostic output involving spectral
rmontuoro Dec 3, 2019
1c0a9ba
Reset to zero instantaneous total moisture tendency
rmontuoro Dec 3, 2019
afb4c67
Merge NOAA-EMC branch 'develop' into develop.
rmontuoro Apr 21, 2020
3004022
Merge NOAA-EMC branch 'develop' into develop.
rmontuoro Apr 24, 2020
617b907
Merge NOAA-EMC branch 'develop' into develop.
rmontuoro May 8, 2020
bdd1131
Merge EMC-NOAA branch 'develop' into develop.
rmontuoro Oct 19, 2020
735eb9e
Temporarily disable filling export fields during
rmontuoro Oct 19, 2020
59cf366
Moving previous commit to a branch.
rmontuoro Oct 19, 2020
6f730ae
Merge EMC branch 'develop' into develop
rmontuoro May 2, 2021
deda93d
Merge NOAA-EMC branch 'develop' into develop
rmontuoro May 11, 2021
bb754cc
Merge EMC branch 'develop' into develop
rmontuoro May 13, 2021
fefc233
Merge NOAA-EMC branch 'develop' into develop
rmontuoro May 21, 2021
0ec3000
Merge NOAA-EMC branch 'develop' into develop
rmontuoro May 21, 2021
3c4d18f
Merge NOAA-EMC branch 'develop' into develop
rmontuoro Jun 3, 2021
a119ee8
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Jul 26, 2021
71a5fd4
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Jul 29, 2021
4c2a7b2
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Aug 9, 2021
80e6ea6
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Aug 10, 2021
f3f4d00
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Aug 12, 2021
24a15fc
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Sep 23, 2021
7b1e88d
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Sep 27, 2021
4f8c34b
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Sep 29, 2021
8f28b16
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Oct 1, 2021
310b4ac
Merge branch 'NOAA-EMC:develop' into develop
rmontuoro Oct 4, 2021
d6bb752
Ensure timestamp of imported fields is set before checking the time.
rmontuoro Sep 23, 2021
fda3a64
Add missing specialization method to verify import fields for run pha…
rmontuoro Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions fv3_cap.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ subroutine SetServices(gcomp, rc)

! specializations required to support 'inline' run sequences
call NUOPC_CompSpecialize(gcomp, specLabel=label_CheckImport, &
specPhaseLabel="phase1", specRoutine=NUOPC_NoOp, rc=rc)
specPhaseLabel="phase1", specRoutine=fv3_checkimport, rc=rc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now the fv3_checkImport will be called once for one integration step with two phases, just as it is called once in "ModelAdvance". One question is: should we also call checkImport for phase2? or maybe it is no necessary as the time stamp does not change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fv3_checkimport should be called at the beginning of the atmosphere's integration step for coupling with external components except GOCART. It is not called before run phase 2 since: (a) the timestamp of imported fields does not change, and (b) GOCART does not require it.

if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return

call NUOPC_CompSpecialize(gcomp, specLabel=label_TimestampExport, &
Expand Down Expand Up @@ -1212,7 +1212,7 @@ subroutine fv3_checkimport(gcomp, rc)
type(ESMF_Clock) :: clock
type(ESMF_Time) :: currTime, invalidTime
type(ESMF_State) :: importState
logical :: timeCheck1,timeCheck2
logical :: isValid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This local variable isValid is used at three places and have three different meanings at all those places. I find it very confusing.
First it is used to indicate whether a field's timestamp is "a valid timestamp", from NUOPC's perspective, and this is the only place this variable should be used with this name.
Second it is reused to check whether a field is not at "invalid time", where an invalid time is defined locally and set to an arbitrary/special value (year 99999999). Why is this necessary? How is this value (99999999) determined?
And finally isValid is reused for a third time to check whether a field is at current time.
If all these checks are necessary, then three logical variables with different names should be used. For example isValid, isNotSpecialValue and isAtCurrentTime, which will improve readability of the code at expense of adding two logical variables (8 bytes).
I wonder if all these check can be combined in just one check that checks whether or not field's time is at current time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use multiple logical variables if needed for clarity. We could also write a single logical function combining all the tests for increased modularity. I don't believe such a function is required elsewhere in the code base, though, and it could lead to confusion in cases where a field timestamp has not been set at all.

type(ESMF_Field),pointer :: fieldList(:)
character(len=128) :: fldname
character(esmf_maxstr) :: msgString
Expand Down Expand Up @@ -1250,25 +1250,32 @@ subroutine fv3_checkimport(gcomp, rc)
call ESMF_FieldGet(fieldList(n), name=fldname, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return

nf = queryImportFields(fldname)
timeCheck1 = NUOPC_IsAtTime(fieldList(n), invalidTime, rc=rc)
! check if import field carries a valid timestamp
call NUOPC_GetTimestamp(fieldList(n), isValid=isValid, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return

if (timeCheck1) then
importFieldsValid(nf) = .false.
! if(mtype==0) print *,'in fv3_checkimport,',trim(fldname),' is set unvalid, nf=',nf,' at time',date(1:6)
else
timeCheck2 = NUOPC_IsAtTime(fieldList(n), currTime, rc=rc)
if (isValid) then
! if timestamp is set, check if it is valid
isValid = .not.NUOPC_IsAtTime(fieldList(n), invalidTime, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return
end if

! store field status in internal array
nf = queryImportFields(fldname)
importFieldsValid(nf) = isValid

if (.not.timeCheck2) then
!TODO: introduce and use INCOMPATIBILITY return codes!!!!
if (isValid) then
! check if field is current
isValid = NUOPC_IsAtTime(fieldList(n), currTime, rc=rc)
if (ESMF_LogFoundError(rcToCheck=rc, msg=ESMF_LOGERR_PASSTHRU, line=__LINE__, file=__FILE__)) return
if (.not.isValid) then
call ESMF_LogSetError(ESMF_RC_ARG_BAD, &
msg="NUOPC INCOMPATIBILITY DETECTED: Import Field not at current time", &
msg="NUOPC INCOMPATIBILITY DETECTED: Import Field " &
// trim(fldname) // " not at current time", &
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have: importFieldsValid(nf) = .false. here too? Or maybe we are now allowing import fields with different time stamp (not at currTime)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could set importFieldsValid(nf) = .false. here as well if we are not planning to abort when a time incompatibility is detected. Currently, we return an ESMF error code that will cause NUOPC to abort.

line=__LINE__, file=__FILE__, rcToReturn=rc)
return
endif
endif
return
end if
end if
write(msgString,'(A,2i4,l3)') "fv3_checkimport "//trim(fldname),n,nf,importFieldsValid(nf)
call ESMF_LogWrite(msgString,ESMF_LOGMSG_INFO,rc=rc)
enddo
Expand Down