-
Notifications
You must be signed in to change notification settings - Fork 18
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.
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
Handmerge of TerribleNews/geos-patch #1795
base: develop
Are you sure you want to change the base?
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.
public MAPL_GenericStateClockOn | ||
public MAPL_GenericStateClockOff | ||
public MAPL_GenericStateClockAdd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_VERIFY
...
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see even more further down now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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: