Properly import coupling fields when running with separate run phases #406
Conversation
large scale rain, and convective rain at the end of each coupling step if coupling with chemistry model.
defining zero and one.
in noah/osu land-surface model subdriver.
band layer cloud optical depths (0.55 and 10 mu channels) to prevent floating invalid errors due to uninitialized optical depth arrays.
coupling array at the beginning of each coupling step if coupled with chemistry model.
the NUOPC Realize phase since it breaks coupling with aerosol component.
This reverts commit 735eb9e.
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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", & |
There was a problem hiding this comment.
Should we have: importFieldsValid(nf) = .false. here too? Or maybe we are now allowing import fields with different time stamp (not at currTime)?
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Description
This PR addresses a coupling issue described in #401.
The
fv3_checkimportmethod is now called also for run phase 1. The method has been refactored to properly check the timestamp of imported fields using updated ESMF API.Issue(s) addressed
Testing
The changes were tested on the Hera/Intel platform.
Dependencies
None