[PULL REQUEST] Add co2 run by TerribleNews · Pull Request #582 · geoschem/geos-chem

TerribleNews · 2021-01-04T02:05:31Z

Companion pull request to geoschem/GCHP#71

This is the submodule I'm least confident about. I had to pull a number of times and each time new conflicts came up.

Updated create run directory to reflect adjoint needs

Main

…add_co2_run

ExtData still had the old symlink names, so ChemDataDir was replaced with ChemDir and so on. runConfig_adj.sh just had a bunch of errors and old script hanging around. And createRunDir.sh wasn't properly replacing values in runConfig_adj.sh either.

lizziel · 2021-03-22T14:04:20Z

I am going to work on bringing in all the adjoint updates this week. @TerribleNews, could you change the target branch to dev?

msulprizio · 2021-03-22T14:43:47Z

could you change the target branch to dev?

I went ahead and did this @lizziel. Anyone can do it by clicking 'Edit' at the top of the PR.

TerribleNews · 2021-03-22T14:51:15Z

I am going to work on bringing in all the adjoint updates this week.

Great!

could you change the target branch to dev?

I went ahead and did this @lizziel. Anyone can do it by clicking 'Edit' at the top of the PR.

I was also hoping to get some changes I committed past the pull request included, but I couldn't figure out how to do it. Should I edit the PR or would you like to check it out first? The additional changes should be real easy, they're all to files I created in the run directory.

msulprizio · 2021-03-22T15:04:38Z

@TerribleNews I believe if you push any changes to your fork's add_co2_run branch, it should automatically update the PR.

TerribleNews · 2021-03-22T15:19:05Z

Oh, then never mind!

lizziel · 2021-03-22T21:00:25Z

Thanks all! @TerribleNews, a few questions/comments:

I noticed a couple instances of #ifdef FALSE in your updates. Are those intentional or are they supposed to be #ifdef ADJOINT?
Some of the changes appear to be debugging changes. Could you browse through the changes (see "Files Changed" tab above) and make sure no temporary debug updates are still present?
It doesn't look like the USE statement for MAPL_CommsMod is not in an ifdef MAPL block. Could you double-check that you don't have any MAPL libraries or other GCHP-specific framework code that might be run in GC-Classic?

Let me know when any edits and other updates are all in. I'll hold off on bringing this in until I get the go-ahead from you.

LiamBindle · 2021-03-22T22:08:19Z

Is it possible to have a runtime setting that enables/disables the adjoint? Does it take a lot longer to compile with the adjoint code? Just thinking it's best we avoid preprocessor definitions unless it is necessary.

TerribleNews · 2021-03-22T23:32:23Z

Thanks all! @TerribleNews, a few questions/comments:

Okay, I'll look into those and let you know when I've sorted them out! Thank you!

Is it possible to have a runtime setting that enables/disables the adjoint? Does it take a lot longer to compile with the adjoint code? Just thinking it's best we avoid preprocessor definitions unless it is necessary.

In fact, there are both compile-time and run-time flags. The preprocessor flags are less about a longer compilation step and more about runtime performance. There are things the adjoint code needs to to during the forward run to prepare for the reverse integration. Our assumption is that most of the GEOS-Chem community will never need to run the adjoint, and there are memory and performance implications for using the adjoint code in forward mode, so we want to keep those declarations and code blocks out of most people's GEOS-Chem. If you want to do adjoint operations, you build a binary with the adjoint compile-time flags and then use that for the forward and reverse integrations with different runtime configurations.

lizziel · 2021-03-29T21:28:28Z

@TerribleNews, do you have a time estimate for when you will get to this? We would like to include the updates in 13.1 which we will be wrapping up in the next couple weeks. If it is not yet ready we could push it to a later release if needed.

TerribleNews · 2021-03-30T00:33:58Z

@TerribleNews, do you have a time estimate for when you will get to this? We would like to include the updates in 13.1 which we will be wrapping up in the next couple weeks. If it is not yet ready we could push it to a later release if needed.

I would really like to get this in for the next minor release cycle, so I will do my best to get it back to you sometime this week.

lizziel · 2021-04-02T12:59:35Z

CMakeLists.txt

 # Print header
 #-----------------------------------------------------------------------------
-get_repo_version(GC_REPO_VERSION ${CMAKE_CURRENT_SOURCE_DIR})
+# get_repo_version(GC_REPO_VERSION ${CMAKE_CURRENT_SOURCE_DIR})


Do you need this commented out?

Ah, yes. I commented this out because if I don't, I get:

CMake Error at src/GCHP_GridComp/GEOSChem_GridComp/geos-chem/CMakeLists.txt:20 (get_repo_version): Unknown CMake command "get_repo_version".

when I run cmake

@LiamBindle has helped me fix this so it's uncommented plus the lines that include the library to avoid the error

lizziel · 2021-04-02T13:00:35Z

GeosCore/hco_interface_gc_mod.F90

+! #ifdef ADJOINT
+!     else
+!     ! Emission, chemistry and dynamics timestep in seconds
+!     HcoState%TS_EMIS = -GET_TS_EMIS()
+!     HcoState%TS_CHEM = -GET_TS_CHEM()
+!     HcoState%TS_DYN  = -GET_TS_DYN()
+!     ! Look into whether we want to change the sign in the body of GET_TS_*()
+!     endif
+! #endif
+


Keep this even though it is commented out?

lizziel · 2021-04-02T13:01:08Z

GeosCore/hco_interface_gc_mod.F90


    USE Time_Mod,        ONLY : Get_Year, Get_Month, Get_Day, GET_DAY_OF_YEAR
    USE Time_Mod,        ONLY : GET_HOUR, GET_MINUTE, GET_SECOND
+    USE MAPL_CommsMod,   ONLY : MAPL_AM_I_ROOT


This will need to be in a MAPL ifdef so that GC-Classic does not execute it

lizziel · 2021-04-02T13:02:20Z

GeosCore/mixing_mod.F90

    USE ERROR_MOD,            ONLY : SAFE_DIV
    USE GET_NDEP_MOD,         ONLY : SOIL_DRYDEP
    USE HCO_Interface_Common, ONLY : GetHcoDiagn
+#ifdef FALSE


Did you intend for this to use FALSE?

lizziel · 2021-04-02T13:02:42Z

GeosCore/mixing_mod.F90

       IF ( .NOT. DryDepSpec .AND. .NOT. EmisSpec ) CYCLE
-
+
+#ifdef FALSE


Same question as before about FALSE.

lizziel · 2021-04-02T13:12:57Z

Interfaces/GCHP/gchp_historyexports_mod.F90


    ! Copy emissions data to MAPL exports via HEMCO
    CALL HCOI_GC_WriteDiagn( Input_Opt, .FALSE., RC )
+    IF ( MAPL_Am_I_Root() ) WRITE(*,*) "Back from HCOI_GC_WriteDiagn, RC = ", RC


Keep but comment out.

lizziel · 2021-04-02T13:13:30Z

run/GCHP/GCHP.rc.template

 IRRADAvrg: 0

-GCHPchem_REFERENCE_TIME: 121000
+GCHPchem_REFERENCE_TIME: 001000


Why is this changing?

I have removed these and the other changes to common run files for now.

lizziel · 2021-04-02T13:13:43Z

run/GCHP/GCHP.rc.template

-RECORD_FREQUENCY: 1680000 
-RECORD_REF_DATE: 20000101
+RECORD_FREQUENCY: 100000000 
+RECORD_REF_DATE: 20140901


Why is this changing?

lizziel · 2021-04-02T13:14:55Z

run/GCHP/input.geos.templates/input.geos.CO2

+%%% EMISSIONS MENU %%%  :
+HEMCO Input file        : HEMCO_Config.rc
+Switches for UCX        :---
+ => Use CH4 emissions?  : F
+ => Set init. strat. H2O: F
+------------------------+------------------------------------------------------


The emissions menu is no longer included in input.geos.

lizziel · 2021-04-02T13:15:12Z

run/GCHP/runConfig.sh.template

-Turn_on_Dry_Deposition=T
-Turn_on_Wet_Deposition=T
+Turn_on_Dry_Deposition=F
+Turn_on_Wet_Deposition=F


TerribleNews · 2021-04-02T15:28:02Z

Thanks @lizziel for the detailed feedback. It's very helpful. In hindsight, several of these were obvious things I should have checked for but since this is my first rodeo I didn't really know to check for them or really how to check for them. I have a better understanding of the interface now, so thank you! I'm working on these and the ones in the other repos and I will let you know when all the fixes are checked in.

lizziel · 2021-04-02T17:30:25Z

Excellent, thanks @TerribleNews.

Removed or commented out extra debug logging Removed #ifdef FALSE sections Repaired CMakeLists.txt so I could uncomment the line that was giving me an error

TerribleNews · 2021-04-03T00:42:57Z

I believe I have dealt with all of @lizziel 's comments on the code. I now have to look into the changes I made to the common run files.

lizziel · 2021-04-06T21:47:43Z

The conflict in mixing_mod.F90 is due to this. It is simple to fix by including ErrorMsg declaration.

<<<<<<< HEAD
    CHARACTER(LEN=255) :: ErrMsg, ErrorMsg, ThisLoc
=======
    CHARACTER(LEN=255) :: ErrMsg, ThisLoc
#ifdef ADJOINT
    LOGICAL                 :: IS_ADJ
#endif
>>>>>>> b1076ea20cf9c5e163485facd2f243dcec3bb0a7

The conflict in runConfig.sh.template is because we have since removed the toggle to turn on/off emissions in input.geos, and thus took it out of runConfig.sh.

Changes to common run files can be maade manually after creation, for now, and conditional change blocks for the adjoint/CO2 run can be added to createRunDir.sh later.

lizziel · 2021-04-07T12:53:46Z

Great, thanks. For the RECORD and drydep/wetdep settings, we can handle setting those to what you have here during run directory creation. The backend for run directory creation is about to change so you can leave these as is and we can do the changes. Is there anything else that needs to change in common run directory file templates (GCHP.rc, runConfig.sh, etc) for CO2?

TerribleNews · 2021-04-07T13:11:33Z

Yes. Also I apologize for shoehorning two different but strongly intertwined features here. It should be possible to separate the CO2 and adjoint run configurations in future, but they kind of got developed together, so it will take some work. For now, if you compile without the ADJOINT and REVERSE_OPERATOR flags (i.e. regular old GCHP), it should still be able to run with the CMSFlux inputs in CO2 only mode for forward. But currently the adjoint code (i.e. complied with -DADJOINT=yes -DREVERSE_OPERATORS=yes) will only work with a CO2 run directory.

In any event, when creating a rundirectory that might be used for adjoint (which is currently the CO2 option in createRunDir.sh) there should be the following added to GCHP.rc

#
# %%% Adjoint variables
#
MODEL_PHASE: FORWARD

This is required for the runConfig.sh and runConfig_adj.sh to have a variable to replace. All the other adjoint options are currently manually defined.

TerribleNews · 2021-04-07T13:18:59Z

Also, thank you @lizziel for guiding me through all this, I really appreciate the extra effort!

lizziel · 2021-04-07T16:20:10Z

No problem. I'm glad you don't mind the pushback. I'm testing GC-Classic and GCHP now (non-adjoint runs) to ensure zero diff. For GC-Classic I had to change part of input_mod.F90. See next review comment.

lizziel · 2021-04-07T16:21:01Z

GeosCore/input_mod.F90

+      ! If we're doing the reverse integration
+      ! multiply all the timesteps by -1 here
+      if (TS_DYN < 0) THEN
+         ! TS_DYN and TS_CHEM should always be set to something valid...?
+         TS_DYN = TS_DYN * -1
+         TS_CHEM = TS_CHEM * -1
+         if (LTURB .or. LCONV) TS_CONV = TS_CONV * -1
+         if (LEMIS .or. LDRYD) TS_EMIS = TS_EMIS * -1
+         ! I'm not sure which flag determins radiation on/off?
+         if (LCHEM) TS_RAD = TS_RAD * -1
+      endif
+
+


For GC-Classic to build and run I needed to change this section to this:

! If we're doing the reverse integration ! multiply all the timesteps by -1 here IF ( TS_DYN < 0 ) THEN TS_CHEM = TS_CHEM * -1 TS_EMIS = TS_EMIS * -1 TS_CONV = TS_CONV * -1 TS_DYN = TS_DYN * -1 TS_RAD = TS_RAD * -1 ENDIF

Otherwise this looks good to go. If you could just push the update I will go ahead and merge. Thanks for your work on this!

lizziel · 2021-04-16T16:23:40Z

Hi @TerribleNews, I have a follow-up question about the changes in this PR. There is now input_opt%is_adjoint, but only if built with ADJOINT. I want to clean up unitconv_mod.F90 so that IS_ADJOINT logical is not passed around. Would Input_Opt%is_adjoint always be .TRUE. if compiled with ADJOINT or did you intend it to be flexible based on a config file setting even when compiling with ADJOINT?

TerribleNews · 2021-04-16T16:42:40Z

Oh yeah, @lizziel, there's some confusing terminology there, so I apologize. I tried to keep things clear and separate where I could, but the field just kind of has some built-in ambiguities. Tldr: the pre-processor ADJOINT flag and input_opt%is_adjoint logical are separate and both required.

A full adjoint integration consists of a forward model run followed by a inverse or adjoint run, i.e., the reverse-time run with the adjoint calculations. In the old GEOS-Chem adjoint, the forward and inverse runs were done with a single execution, but we decided in this case that we would allow the forward and inverse runs to be two separate executions. In doing it this way, we had the option of creating two binaries (e.g., gchp and gchp_adj) or one with a runtime option. We chose the latter option, so both the forward and inverse runs execute gchp but with different values in the MODEL_PHASE: variable in GCHP.rc.

In addition, the forward model run for an adjoint integration requires extra operations from the non-adjoint-enabled forward model, so executing a binary compiled with -DADJOINT but MODEL_PHASE: FORWARD (i.e., input_opt%is_adjoint = .false.) is different from just running gchp compiled without -DADJOINT.

The actual potential redundancy is having two pre-processor flags, -DADJOINT and -DREVERSE_OPERATORS, but I left them because they could be separate changes to the code (although the way they are currently setup, the only #ifdef REVERSE_OPERATORS blocks are nested inside #ifdef ADJOINT blocks).

Summary: -DADJOINT means adjoint-enabled binary and input_opt%is_adjoint means "are we running the forward or inverse integration?

lizziel · 2021-04-16T17:03:17Z

Got it, thanks! My change will be to always define input_opt%is_adjoint (whether or not built with -DADJOINT) and pass input_opt to unit conversion subroutines rather than local logical IS_ADJOINT. I'll keep the ADJOINT ifdefs as is in unitconv_mod.F90, but replace IS_ADJOINT logical with input_opt%is_adjoint within the conversions. Let me know if you see any issues with this for what you are doing, but I think it should work out.

Colin Lee and others added 11 commits October 13, 2020 10:51

Added base files for CO2 simulation type

7515c06

Forward and reverse models compile and run

e6e2ebd

Updated create run directory to reflect adjoint needs

83d328c

Merge pull request #1 from TerribleNews/origin/add_co2_run

cd488b8

Updated create run directory to reflect adjoint needs

Merge pull request #2 from geoschem/main

8086fe4

Main

Added base files for CO2 simulation type

a9f305e

Forward and reverse models compile and run

2f675c7

Updated create run directory to reflect adjoint needs

f358e6a

Fixed compile errors in adjoint code

3502d06

Merge branch 'add_co2_run' of github.com:TerribleNews/geos-chem into …

a62f440

…add_co2_run

Moved adjoint code into #ifdef blocks

32c723f

yantosca added the never stale Never label this issue as stale label Jan 13, 2021

yantosca changed the title ~~Add co2 run~~ [PULL REQUEST] Add co2 run Jan 13, 2021

msulprizio added this to the 13.1.0 milestone Feb 27, 2021

LiamBindle assigned LiamBindle and lizziel and unassigned LiamBindle Mar 3, 2021

msulprizio added category: Feature Request New feature or request and removed never stale Never label this issue as stale labels Mar 3, 2021

msulprizio mentioned this pull request Mar 3, 2021

[DISCUSSION] The status of CO2-only mode? geoschem/GCHP#51

Closed

Fixed errors in CO2 files.

81a3a0e

ExtData still had the old symlink names, so ChemDataDir was replaced with ChemDir and so on. runConfig_adj.sh just had a bunch of errors and old script hanging around. And createRunDir.sh wasn't properly replacing values in runConfig_adj.sh either.

msulprizio changed the base branch from main to dev March 22, 2021 14:42

Updated runscripts MPI configuration to match runConfig.sh

5891c0a

lizziel reviewed Apr 2, 2021

View reviewed changes

Responses to Lizzie's github pull request feedback

b1076ea

Removed or commented out extra debug logging Removed #ifdef FALSE sections Repaired CMakeLists.txt so I could uncomment the line that was giving me an error

Colin Lee added 2 commits April 6, 2021 15:46

Fixed merge conflict in mixing_mod

d298d16

Undo changes to common run files, update input.geos.CO2

4c0a282

Changes to common run files can be maade manually after creation, for now, and conditional change blocks for the adjoint/CO2 run can be added to createRunDir.sh later.

lizziel reviewed Apr 7, 2021

View reviewed changes

Colin Lee and others added 2 commits April 7, 2021 10:22

Removed conditions causing GCC to fail

48919c2

Merge branch 'dev' into add_co2_run

e34e0eb

lizziel merged commit d372197 into geoschem:dev Apr 7, 2021

		IF ( .NOT. DryDepSpec .AND. .NOT. EmisSpec ) CYCLE


		#ifdef FALSE

Conversation

TerribleNews commented Jan 4, 2021

Uh oh!

lizziel commented Mar 22, 2021

Uh oh!

msulprizio commented Mar 22, 2021

Uh oh!

TerribleNews commented Mar 22, 2021

Uh oh!

msulprizio commented Mar 22, 2021

Uh oh!

TerribleNews commented Mar 22, 2021

Uh oh!

lizziel commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiamBindle commented Mar 22, 2021

Uh oh!

TerribleNews commented Mar 22, 2021

Uh oh!

lizziel commented Mar 29, 2021

Uh oh!

TerribleNews commented Mar 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerribleNews Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerribleNews commented Apr 2, 2021

Uh oh!

lizziel commented Apr 2, 2021

Uh oh!

TerribleNews commented Apr 3, 2021

Uh oh!

lizziel commented Apr 6, 2021

Uh oh!

lizziel commented Apr 7, 2021

Uh oh!

TerribleNews commented Apr 7, 2021

Uh oh!

TerribleNews commented Apr 7, 2021

Uh oh!

lizziel commented Apr 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizziel commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TerribleNews commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizziel commented Apr 16, 2021

Uh oh!

lizziel commented Mar 22, 2021 •

edited

Loading

TerribleNews Apr 2, 2021 •

edited

Loading

lizziel commented Apr 16, 2021 •

edited

Loading

TerribleNews commented Apr 16, 2021 •

edited

Loading