Forward and reverse models compile and run

[TerribleNews]

geoschem/GCHP#71

To get the model to run in reverse and appropriately output diagnostics, a number of changes had to be made the main run loop and the history clock and alarm system. I was also found what I believe to be a but in the ESMF C library code for alarms in reverse mode:

diff --git a/src/Infrastructure/TimeMgr/src/ESMCI_Clock.C b/src/Infrastructure/TimeMgr/src/ESMCI_Clock.C
index bceb778..97eb4ef 100644
--- a/src/Infrastructure/TimeMgr/src/ESMCI_Clock.C
+++ b/src/Infrastructure/TimeMgr/src/ESMCI_Clock.C
@@ -566,6 +566,7 @@ int Clock::count=0;
  #define ESMC_METHOD "ESMCI::Clock::advance()"

     int rc = ESMF_SUCCESS;
+    bool userChangedDirLocal;

     if (this == ESMC_NULL_POINTER) {
       ESMC_LogDefault.MsgFoundError(ESMC_RC_PTR_NULL,
@@ -638,11 +639,16 @@ int Clock::count=0;
                                     ringingAlarmList1stElementPtr);
     }

+    // I think there's a bug where each alarm sets userChangedDirection to false
+    // so only the first alarm knows if the user changed direction
+    userChangedDirLocal = this->userChangedDirection;
+
     // traverse alarm list (i) for ringing alarms (j)
     for(int i=0, j=0; i<alarmCount; i++) {
       int rc;
       bool ringing;

+      this->userChangedDirection = userChangedDirLocal;
       // check each alarm to see if it's time to ring
       ringing = alarmList[i]->Alarm::checkRingTime(&rc);

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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

[TerribleNews]

I am unable to view the logs from the automated tests. Is there a way to trigger them to run again? Is that what is holding up this part of the PR set?

[lizziel]

You can ignore those as they are GMAO tests. I removed the relevant .github files in a recent commit to prevent them from being triggered. That is in the gchp/dev branch so your PR on gchp/main still has them.

[lizziel]

Are you planning on submitting these updates to GMAO to include in a future MAPL version? We are trying to keep our MAPL as close as possible to the upstream, and clearly mark anything that differs with comments that it is for GCHP. This helps with merge conflicts in the future during MAPL version updates.

[lizziel]

I've merged in all your CO2/adjoint PRs except for this one. My preference is for you to make a PR for these updates at https://github.com/GEOS-ESM/MAPL, and update this one to wrap all changes in ADJOINT ifdefs. This would make it very clear what has been added for the adjoint and ensure deactivation for the forward model. I'd the defer to GMAO on what they want to include in an official MAPL release that we would merge into our fork later. Does this sound okay?

[TerribleNews]

I'm not sure I follow completely. Are you proposing ditching this PR and me submitting a PR to GMAO and then these changes come into the geoschem/MAPL repo once they've been accepted at GMAO? Or are you proposing I basically make duplicate PRs here and at GMAO?

[TerribleNews]

These and the subsequent changes are (I don't think) adjoint specific. I was having a lot of weird issues with files with + signs at the begining which was supposed to signify something to MAPL but it was trying to open them including the + and failing. I'm not sure if we've stopped using the + in files, but if so, I can remove these changes.

[lizziel]

Right, it looks like most of the changes are not adjoint-specific. They may possibly affect GCHP as well and may or may not go in a direction GMAO agrees with. If you need these in the MAPL included in GCHP 13.1 within the next few weeks then I think the changes could still come in, but with ADJOINT ifdefs where functionality changes, and adjoint comments where it does not but the code is added purely for the adjoint. More generally I think a conversation about the changes should be in the GEOS-ESM space as a PR so the MAPL developers can comment and bring in to an official MAPL version. If they reject anything then we can discuss why and what to do about it.

[TerribleNews]

Yes, that is all fair. This will require some more thinking/testing/reworking on my end. All the MAPL stuff was pretty stickly. And as I mentioned in the original PR, I had to make a small change to the EMSF underlying c code (not the ESMA_cmake, but the source I downloaded for ESMF 8.0.0). I'll try to get back to you early next week.

[lizziel]

Okay, sounds good. I definitely recommend getting in touch with the ESMF developers as well if you had to change ESMF. I've contacted them about a few things and they were very responsive. They want their code to work for everyone. Now that we use ESMF externally we do not recommend custom updates.

Heads up we just switched from MAPL v2.2.7 to v2.6.3. The new MAPL version is currently in branch gchp/dev. I don't think it should be hard for your updates to be adapted to it. The main things are the Cap grid comp files moved from base to directory gridcomps/Cap and the History grid comp files moved from base to directory gridcomps/History. Your update has a merge conflict in MAPL_Generic.F90 too eventhough that file did not move. I suspect if you make your Cap and History changes in the appropriate new locations then the remaining conflict in MAPL_Generic.F90 will be viewable from GitHub (there's a button to the right of "This branch has conflicts that must be resolved" message that shows you the conflicts if they aren't extensive).

[TerribleNews]

Still working on this. Hope to have an update for you soon.

[TerribleNews]

Hi @lizziel while trying to put together a pull request for the GMAO team I found that there are some large differences in our version of MAPL_CapGridComp.F90 and theirs. This poses a problem because I had to create a step_reverse subroutine that is a backwards version of step which appears to have changed quite a bit from our step. What do you think is the best course of action here?

[lizziel]

Thanks for bringing this up. How about this for a plan:

• I will take a look at the differences between ours and the upstream to refamiliarize myself with them. I had to make some updates to Cap at some point to avoid using their command line tool FLAP. Definitely worth revisiting this.
• Let's discuss both those existing differences and your proposed updates at an upcoming MAPL meeting with GMAO.
• Meanwhile, if your step updates fit in with what we have in our MAPL fork branch gchp/dev (soon to be merged into gchp/main) you can do a PR on geoschem/mapl with them. I assume the forward model will not call step_reverse so this seems safe, but let me know if that is not the case, i.e. other things are changed that potentially impact GCHP.

Sound ok?

[TerribleNews]

Which branch should I be aiming for on GEOS-ESM/MAPL? I was assuming develop but if I can send a PR to main, maybe that will be less of a change.

As an aside, I have submitted the PR to esmf-org/esmf, but that change is also required for GCHP adjoint; for now my plan is to give it to anyone running the adjoint as a patch to ESMF 8.0.0.

I believe this PR is now the minimal set of changes to geoschm/mapl required to get GCHP adjoint working, plus a bunch of fixes for issues I had encountered with config files with plus signs.

[TerribleNews]

Sorry, I see that there are still conflicts but the "resolve conflicts" button next to them is greyed out for me. Are there a lot of them? Let me know if you want me to rebase my changes before you proceed.

[lizziel]

The conflicts are because two of the files moved. See my earlier comment about the update to MAPL v2.6.

[lizziel]

@TerribleNews, could you confirm this PR is now superceded by #12?

[TerribleNews]

Yes, this is superseded by #12