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

Run in reverse to support adjoint calculation #12

Merged
merged 2 commits into from Oct 6, 2021

Conversation

TerribleNews
Copy link

Description

Added functionality to run MAPL_CapGridCap in reverse and supporting changes to History GridComp to fix alarms in reverse.

Related Issue

This is a new feature

Motivation and Context

This is necessary to allow for the adjoint calculations to be run in reverse mode.

How Has This Been Tested?

This shouldn't change anything unless ADJOINT mode is turned on in GCHP.rc. Otherwise everything should behave exactly as before. I have compiled and run on pleiades but I have done limited testing. I'd be happy to perform more tests as requested by the GCST.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] 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:

  • [ x] I have tested this change with a run of GCHP

Colin Lee added 2 commits July 19, 2021 17:24
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.
@lizziel lizziel self-assigned this Jul 28, 2021
@lizziel lizziel added the enhancement New feature or request label Jul 28, 2021
@lizziel
Copy link

lizziel commented Jul 28, 2021

Thanks @TerribleNews. I will bring this into the next GEOS-Chem version (13.3).

@TerribleNews
Copy link
Author

Thanks @TerribleNews. I will bring this into the next GEOS-Chem version (13.3).

Great news! Thank you @lizziel. I have also started merging these same changes into the develop branch of GEOS-ESM/MAPL and I should be able to fire that pull request off this week sometime.

@lizziel
Copy link

lizziel commented Aug 26, 2021

@TerribleNews, are there are any PRs on other repos you plan on making in the near future to go along with these changes for GCHP adjoint?

@TerribleNews
Copy link
Author

There's geoschem/HEMCO#97 and geoschem/geos-chem#797, but apart from that I don't have anything right now. I know there will be more adjoint code changes over the next few weeks, so it depends on the timeline; what do you mean by "near future"?

@lizziel
Copy link

lizziel commented Aug 30, 2021

what do you mean by "near future"?

We are planning the next version code freeze for last week in September. All three of these PRs can be brought in before then.

@LiamBindle LiamBindle changed the base branch from gchp/main to gchp/dev October 4, 2021 20:46
@LiamBindle LiamBindle self-requested a review October 4, 2021 20:46
Copy link

@LiamBindle LiamBindle left a comment

Choose a reason for hiding this comment

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

Looks good to me. @TerribleNews could you confirm whether or not we should retain the commented-out blocks?

I'll merge and test with the associated PRs after that.

Comment on lines +1099 to +1105
! call ESMF_ClockGetAlarmList(cap%clock, ESMF_ALARMLIST_ALL, &
! alarmList=alarm, alarmCount=alarmCount, rc = status )
! _VERIFY(STATUS)

! call ESMF_ClockGetAlarmList(cap%clock_hist, ESMF_ALARMLIST_ALL, &
! alarmList=hist_alarm, alarmCount=hist_alarmCount, rc = status )
! _VERIFY(STATUS)

Choose a reason for hiding this comment

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

Is this meant to be left commented out? If so, is there a reason to retain it?

Comment on lines +1109 to +1111
! WRITE(*,1003) 'clock', alarmCount

! WRITE(*,1003) 'clock_hist', hist_alarmCount

Choose a reason for hiding this comment

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

Same here (do these need to be retained?)

Comment on lines +1593 to +1596
! call ESMF_GridCompWriteRestart(this%gcs(this%root_id), importstate = this%child_imports(this%root_id), &
! exportstate = this%child_exports(this%root_id), &
! clock = this%clock_hist, userrc = status)
! _VERIFY(status)

Choose a reason for hiding this comment

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

Should we retain this?

Comment on lines +1194 to +1201
! if(list(n)%mode == "instantaneous" .or. list(n)%ForceOffsetZero) then
! sec = 0
! else
! IntState%average(n) = .true.
! sec = MAPL_nsecf(list(n)%acc_interval) / 2
! endif
! call ESMF_TimeIntervalSet( INTSTATE%STAMPOFFSET(n), S=sec, rc=status )
! _VERIFY(STATUS)

Choose a reason for hiding this comment

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

Should we retain this?

@TerribleNews
Copy link
Author

I think it's safe to drop all those comments. They're mostly debug printing and one is about a part of the clock system that has changed. Thanks!

@LiamBindle
Copy link

Sounds good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants