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.

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Nov 8, 2022

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

Colin Lee and others added 7 commits October 31, 2021 16:48
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 mathomp4 added the 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. label Nov 8, 2022
@mathomp4 mathomp4 requested a review from a team as a code owner November 8, 2022 19:07
@mathomp4 mathomp4 self-assigned this Nov 8, 2022
@mathomp4
Copy link
Member Author

mathomp4 commented Nov 8, 2022

Probably need someone like @tclune or @atrayano to look at this. Isn't building at all.

Comment on lines +180 to +182
public MAPL_GenericStateClockOn
public MAPL_GenericStateClockOff
public MAPL_GenericStateClockAdd
Copy link
Collaborator

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.

Comment on lines +535 to +544
! 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)

Copy link
Collaborator

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:

Suggested change
! 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
Copy link
Collaborator

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)
Copy link
Collaborator

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
Copy link
Collaborator

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)
Copy link
Collaborator

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)
Copy link
Collaborator

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)
Copy link
Collaborator

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?

Comment on lines +1322 to +1326
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
Copy link
Collaborator

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?

Copy link
Contributor

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)

Copy link
Member Author

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.

Comment on lines +1369 to +1373
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
Copy link
Collaborator

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.

Copy link
Collaborator

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!

Copy link
Collaborator

@tclune tclune left a 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.

@TerribleNews
Copy link

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?

@mathomp4
Copy link
Member Author

@TerribleNews have you had a chance to work on this?

@stale
Copy link

stale bot commented Feb 10, 2023

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.

@stale stale bot added the ❄️ Stale This issue has been marked stale label Feb 10, 2023
@mathomp4 mathomp4 added the ⌛ Long Term Long term issues label Feb 13, 2023
@stale stale bot removed the ❄️ Stale This issue has been marked stale label Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch. ⌛ Long Term Long term issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants