Handmerge of TerribleNews/geos-patch #1795
Conversation
This commit adds code to allow the CapGridComp to run in reverse while maintaining the ability to trigger history alarms. This may also require a small bugfix in the ESMF 8.0 code.
GMAO doesn't like preprocessor defines so I removed most of them but had forgotten to do the step_reverse function. This change fixes that.
I had made some changes in previous tests to get the reverse model running but forgot to remove these two.
mathomp4
added
the
0 Diff
| public MAPL_GenericStateClockOn | ||
| public MAPL_GenericStateClockOff | ||
| public MAPL_GenericStateClockAdd |
There was a problem hiding this comment.
I do not understand why this change requires making these procedures public.
| ! pass REVERSE_TIME resource to history and root config | ||
| call MAPL_GetResource(MAPLOBJ, reverseTime, "REVERSE_TIME:", default = 0, rc = status) | ||
| _VERIFY(status) | ||
|
|
||
| call MAPL_ConfigSetAttribute(cap%cf_root, value=reverseTime, Label="REVERSE_TIME:", rc=status) | ||
| _VERIFY(STATUS) | ||
|
|
||
| call MAPL_ConfigSetAttribute(cap%cf_hist, value=reverseTime, Label="REVERSE_TIME:", rc=status) | ||
| _VERIFY(STATUS) | ||
|
|
There was a problem hiding this comment.
Update to improved macro for error handling:
| ! pass REVERSE_TIME resource to history and root config | |
| call MAPL_GetResource(MAPLOBJ, reverseTime, "REVERSE_TIME:", default = 0, rc = status) | |
| _VERIFY(status) | |
| call MAPL_ConfigSetAttribute(cap%cf_root, value=reverseTime, Label="REVERSE_TIME:", rc=status) | |
| _VERIFY(STATUS) | |
| call MAPL_ConfigSetAttribute(cap%cf_hist, value=reverseTime, Label="REVERSE_TIME:", rc=status) | |
| _VERIFY(STATUS) | |
| ! pass REVERSE_TIME resource to history and root config | |
| call MAPL_GetResource(MAPLOBJ, reverseTime, "REVERSE_TIME:", default = 0, _RC) | |
| call MAPL_ConfigSetAttribute(cap%cf_root, value=reverseTime, Label="REVERSE_TIME:", _RC) | |
| call MAPL_ConfigSetAttribute(cap%cf_hist, value=reverseTime, Label="REVERSE_TIME:", _RC) | |
|
|
||
| ! WRITE(*,1003) 'clock_hist', hist_alarmCount | ||
|
|
||
| WRITE(*,1001) cap%nsteps |
There was a problem hiding this comment.
Do not use FORMAT statements if possible. Inline the format.
| ! Advance the Clock before running History and Record | ||
| ! --------------------------------------------------- | ||
| call ESMF_ClockAdvance(cap%clock, rc = status) | ||
| _VERIFY(STATUS) |
There was a problem hiding this comment.
Eliminate all _VERIFY in this block in favor of _RC as described above.
| default=0, rc = status) | ||
| _VERIFY(STATUS) | ||
|
|
||
| if ( reverse_time == 1 ) then |
There was a problem hiding this comment.
Move the statements inside this conditional into a new procedure (with appropriate arguments). Then just call that procedure here.
| ! ----------------------------- | ||
|
|
||
| call ESMF_VMBarrier(this%vm, rc = status) | ||
| _VERIFY(STATUS) |
| call ESMF_GridCompRun(this%gcs(this%history_id), importstate=this%child_imports(this%history_id), & | ||
| exportstate = this%child_exports(this%history_id), & | ||
| clock = this%clock_hist, userrc = status) | ||
| _VERIFY(status) |
There was a problem hiding this comment.
Need to check rc and userrc. Probably need an additional local variable for the latter. We usually spell it userRC
|
|
||
| end subroutine | ||
|
|
||
| subroutine print_throughput(rc) |
There was a problem hiding this comment.
The inner procedures here seem to serve a similar purpose to similarly named procedures elsewhere. Would a bit of refactoring allow the same procedures to be used in both places? I.e., can we eliminate some duplication with a modest effort?
| read(tmpstring( 1: 4),'(i4.4)') year | ||
| read(tmpstring( 6: 7),'(i2.2)') month | ||
| read(tmpstring( 9:10),'(i2.2)') day | ||
| read(tmpstring(12:13),'(i2.2)') hour | ||
| read(tmpstring(15:16),'(i2.2)') minute |
There was a problem hiding this comment.
@bena-nasa Isn't there a way to get these integers directly from ESMF rather than needing to process a string buffer?
There was a problem hiding this comment.
Yes, you can query ESMF_Time for all of these (I am not 100% sure if we need separate calls, or we could get them with a single call)
There was a problem hiding this comment.
We do things like this:
call ESMF_TimeGet(fileTime,yy=iyr,mm=imm,dd=idd,h=ihr,m=imn,s=isc,_RC)in MAPL. So this could do the same.
| read(timestring( 1: 4),'(i4.4)') year | ||
| read(timestring( 6: 7),'(i2.2)') month | ||
| read(timestring( 9:10),'(i2.2)') day | ||
| read(timestring(12:13),'(i2.2)') hour | ||
| read(timestring(15:16),'(i2.2)') minute |
There was a problem hiding this comment.
If we cannot get these directly, we need a helper subroutine. This logic appears 3 times in this PR.
There was a problem hiding this comment.
I see even more further down now!
There was a problem hiding this comment.
@TerribleNews First apologies that we lost track of your PR. Second, we have resubmitted your PR because we needed to reconcile your branch with more recent updates. And 3rd, please take a look at the various comments/requests I have made for this PR.
Because of our permissions, you may need to fetch this branch into your clone and then ask us to pull back into this branch.
@sdeastham Just letting you know we've started acting on this.
|
I'm glad this is finally happening, thanks for all the notes. I will try to apply these changes and then do I will have to submit a new pull request? |
|
@TerribleNews have you had a chance to work on this? |
|
This issue has been automatically marked as stale because it has not had recent activity. If there are no updates within 7 days, it will be closed. You can add the "long term" tag to prevent the Stale bot from closing this issue. |
NOTE
This is a hand-merge version of #1166 by @TerribleNews due to the age of that PR versus newer MAPL. I'll repeat all the text from there. Unfortunately, this means @TerribleNews might not get the commit credit!
Description
This change adds a time loop to CapGridComp run the clock forward before the time loop that actually executes the grid components. There are also some small changes to handle issues with alarms not ringing properly in reverse.
Related Issue
Fixes #1144
Motivation and Context
This feature is to support to adjoint development in GCHP, which requires running the model in reverse. There may be other advantages to a generic capability to run MAPL in reverse as well.
This will be my first merge request to the team, so I am submitting mostly to start the conversation and get guidance / feedback.
How Has This Been Tested?
Unfortunately, because GCHP uses a fork of MAPL it is non-trivial to test code changes with the latest version. All these changes are patterned after changes that have been tested in GCHP using the verison of MAPL that is there, which is based on 2.6.3. Is there a way for me to test these exact changes?
Types of changes
Checklist: