Skip to content

Conversation

@lwcugb
Copy link

@lwcugb lwcugb commented Dec 20, 2024

This pull request adds the MEGAN2.1 BVOC emission scheme based on its implementation in HEMCO.

@bbakernoaa
Copy link
Collaborator

@lwcugb overall I think this looks good. There isn't anything too surprising here. I am wondering if we should add methods for to "accumulate" the values for the historical met data or perhaps store this in the DiagState? @zmoon what do you think?

Copy link
Collaborator

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

Just a few initial comments so far.

@lwcugb could you address the compiler warnings associated with your changes? Let me know if you have questions.

@lwcugb
Copy link
Author

lwcugb commented Jan 15, 2025

Just a few initial comments so far.

@lwcugb could you address the compiler warnings associated with your changes? Let me know if you have questions.

@zmoon Do you mean the warning below? I am not sure how it happens. Could you help me get rid of it?

CMake Deprecation Warning at CMakeLists.txt:1 (cmake_minimum_required): Compatibility with CMake < 3.10 will be removed from a future version of CMake. Update the VERSION argument value or use a ... suffix to tell CMake that the project does not need compatibility with older versions.

@zmoon
Copy link
Collaborator

zmoon commented Jan 15, 2025

@zmoon Do you mean the warning below? I am not sure how it happens. Could you help me get rid of it?

Not that one, we can address that later, it's not related to your PR.

I was talking about the warnings from the compiler:

For example with gfortran-11
/home/zmoon/git/CATChem/src/core/config_mod.F90:1420:46:                                                                                                                                                                                                                                                                                                                                                                         1420 |       INTEGER                      :: a_int(4)
      |                                              1
Warning: Unused variable 'a_int' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1432:46:

 1432 |       CHARACTER(LEN=QFYAML_StrLen) :: a_str(2)
      |                                              1
Warning: Unused variable 'a_str' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1414:39:

 1414 |       INTEGER                      :: C
      |                                       1
Warning: Unused variable 'c' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1413:39:

 1413 |       INTEGER                      :: N
      |                                       1
Warning: Unused variable 'n' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1425:52:

 1425 |       CHARACTER(LEN=255)           :: thisLoc,  nLev
      |                                                    1
Warning: Unused variable 'nlev' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1412:46:

 1412 |       INTEGER                      :: nSubStrs
      |                                              1
Warning: Unused variable 'nsubstrs' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1431:53:

 1431 |       CHARACTER(LEN=255)           :: subStrs(MAXDIM)
      |                                                     1
Warning: Unused variable 'substrs' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1428:43:

 1428 |       CHARACTER(LEN=QFYAML_StrLen) :: v_str
      |                                           1
Warning: Unused variable 'v_str' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1423:56:

 1423 |       CHARACTER(LEN=10)            :: xMin_Str, xMax_Str
      |                                                        1
Warning: Unused variable 'xmax_str' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1423:46:

 1423 |       CHARACTER(LEN=10)            :: xMin_Str, xMax_Str
      |                                              1
Warning: Unused variable 'xmin_str' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1424:56:

 1424 |       CHARACTER(LEN=10)            :: yMin_Str, yMax_Str
      |                                                        1
Warning: Unused variable 'ymax_str' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/core/config_mod.F90:1424:46:

 1424 |       CHARACTER(LEN=10)            :: yMin_Str, yMax_Str
      |                                              1
Warning: Unused variable 'ymin_str' declared at (1) [-Wunused-variable]

/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:900:10:

  900 |       IF ( CMLAI == PMLAI ) THEN !(i.e. LAI stays the same)
      |          1
Warning: Equality comparison for REAL(4) at (1) [-Wcompare-reals]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1141:42:

 1141 |       real(fp) :: PAC_DAILY, PHI, BBB, AAA, GAMMA_P_STANDARD
      |                                          1
Warning: Unused variable 'aaa' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1141:37:

 1141 |       real(fp) :: PAC_DAILY, PHI, BBB, AAA, GAMMA_P_STANDARD
      |                                     1
Warning: Unused variable 'bbb' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1162:22:

 1162 |       real(fp) :: EA2L, EA1L, GAMMA_T_LD_SUN, GAMMA_T_LD_SHADE
      |                      1
Warning: Unused variable 'ea2l' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1141:60:

 1141 |       real(fp) :: PAC_DAILY, PHI, BBB, AAA, GAMMA_P_STANDARD
      |                                                            1
Warning: Unused variable 'gamma_p_standard' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1147:37:

 1147 |       real(fp) :: GAMMA_T_LD_STANDARD
      |                                     1
Warning: Unused variable 'gamma_t_ld_standard' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1141:32:

 1141 |       real(fp) :: PAC_DAILY, PHI, BBB, AAA, GAMMA_P_STANDARD
      |                                1
Warning: Unused variable 'phi' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1156:38:

 1156 |       real(fp) :: T_Leaf_Wind_Shade(5)
      |                                      1
Warning: Unused variable 't_leaf_wind_shade' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:1155:36:

 1155 |       real(fp) :: T_Leaf_Wind_Sun(5)
      |                                    1
Warning: Unused variable 't_leaf_wind_sun' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:665:39:

  665 |    subroutine GET_GAMMA_T_LD_C(T, PT_15, PT_24, CT1, CEO, T_Leaf_Int, T_Leaf_Temp, GAMMA_T_LD_C )
      |                                       1
Warning: Unused dummy argument 'pt_15' at (1) [-Wunused-dummy-argument]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:615:32:

  615 |       real(fp), parameter :: z0s = 0.0008467 !< ideal roughness length of soil
      |                                1
Warning: Unused parameter 'z0s' declared at (1) [-Wunused-parameter]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:556:43:

  556 |    subroutine GET_GAMMA_T_LD(T, PT_15, PT_1, CT1, CEO, GAMMA_T_LD)
      |                                           1
Warning: Unused dummy argument 'pt_1' at (1) [-Wunused-dummy-argument]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:529:29:

  529 |       real(fp) :: L_T, L_PT_T    !<
      |                             1
Warning: Unused variable 'l_pt_t' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:149:10:

  149 |       use Error_Mod,     Only : CC_SUCCESS, CC_FAILURE, CC_Error
      |          1
Warning: Unused parameter 'cc_success' which has been explicitly imported at (1) [-Wunused-parameter]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:148:10:

  148 |       use precision_mod, only : fp, ZERO
      |          1
Warning: Unused parameter 'zero' which has been explicitly imported at (1) [-Wunused-parameter]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_bvoc_common_mod.F90:911:63:

  911 |             FNEW = ( TI / DBTWN ) * ( 1.0_fp -  PMLAI / CMLAI )
      |                                                               ^
Warning: 'ti' may be used uninitialized in this function [-Wmaybe-uninitialized]
[ 68%] Building Fortran object src/process/bvoc/CMakeFiles/CATChem_process_bvoc.dir/ccpr_scheme_meganv21_mod.F90.o
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_scheme_meganv21_mod.F90:90:10:

   90 |       USE Constants,     Only : g0, PI_180 ! Example to pull in a constant from the CONSTANTS MODULE < Modify as needed >
      |          1
Warning: Unused parameter 'g0' which has been explicitly imported at (1) [-Wunused-parameter]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_scheme_meganv21_mod.F90:166:33:

  166 |       INTEGER             :: K, S !,DOY  !canopy add and below
      |                                 1
Warning: Unused variable 's' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/ccpr_scheme_meganv21_mod.F90:91:10:

   91 |       use precision_mod, only : fp, ZERO  ! Example to pull in a precision from the PRECISION MODULE < Modify as needed >
      |          1
Warning: Unused parameter 'zero' which has been explicitly imported at (1) [-Wunused-parameter]
[ 70%] Building Fortran object src/process/bvoc/CMakeFiles/CATChem_process_bvoc.dir/CCPr_BVOC_Mod.F90.o
/home/zmoon/git/CATChem/src/process/bvoc/CCPr_BVOC_Mod.F90:201:29:

  201 |       integer            :: c, s, cat_bvoc_idx
      |                             1
Warning: Unused variable 'c' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/CCPr_BVOC_Mod.F90:201:46:

  201 |       integer            :: c, s, cat_bvoc_idx
      |                                              1
Warning: Unused variable 'cat_bvoc_idx' declared at (1) [-Wunused-variable]
/home/zmoon/git/CATChem/src/process/bvoc/CCPr_BVOC_Mod.F90:179:81:

  179 |    SUBROUTINE CCPr_BVOC_Run( MetState, EmisState, DiagState, BvocState, ChemState, RC )
      |                                                                                 1
Warning: Unused dummy argument 'chemstate' at (1) [-Wunused-dummy-argument]
/home/zmoon/git/CATChem/src/process/bvoc/CCPr_BVOC_Mod.F90:179:59:

  179 |    SUBROUTINE CCPr_BVOC_Run( MetState, EmisState, DiagState, BvocState, ChemState, RC )
      |                                                           1
Warning: Unused dummy argument 'diagstate' at (1) [-Wunused-dummy-argument]
/home/zmoon/git/CATChem/src/process/bvoc/CCPr_BVOC_Mod.F90:40:47:

   40 |    SUBROUTINE CCPR_BVOC_Init( Config, ChemState, EmisState, BvocState, RC )
      |                                               1
Warning: Unused dummy argument 'chemstate' at (1) [-Wunused-dummy-argument]

/home/zmoon/git/CATChem/tests/test_bvoc.F90:129:32:

  129 |    subroutine print_info(Config_, BvocState_, MetState_, title_)
      |                                1
Warning: Unused dummy argument 'config_' at (1) [-Wunused-dummy-argument]

@lwcugb
Copy link
Author

lwcugb commented Jan 15, 2025

@zmoon Do you mean the warning below? I am not sure how it happens. Could you help me get rid of it?

Not that one, we can address that later, it's not related to your PR.

I was talking about the warnings from the compiler:

For example with gfortran-11

Oh ok. It seems to have some unused variables. I can clean them. But I guess we should keep the unused 'chemstate' and 'diagstate' for future use?

@zmoon
Copy link
Collaborator

zmoon commented Jan 15, 2025

Oh ok. It seems to have some unused variables. I can clean them. But I guess we should keep the unused 'chemstate' and 'diagstate' for future use?

If it's something you are reasonably sure may be used in the future, you can comment the declaration out instead of fully removing it (but still remove it from arguments if necessary to resolve the warning).

And if you have "equality comparison for real" warning, you can use the rae function from precision_mod to do the comparison instead.

@lwcugb lwcugb requested a review from zmoon January 22, 2025 17:17
@zmoon zmoon force-pushed the feature/catchem_begins branch from 53ad0f7 to 12d57c6 Compare January 22, 2025 19:08
@zmoon zmoon force-pushed the feature/catchem_begins branch from 12d57c6 to 1bda156 Compare January 22, 2025 19:10
Copy link
Collaborator

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the warnings!

@lwcugb
Copy link
Author

lwcugb commented Jan 22, 2025

@lwcugb overall I think this looks good. There isn't anything too surprising here. I am wondering if we should add methods for to "accumulate" the values for the historical met data or perhaps store this in the DiagState? @zmoon what do you think?

@bbakernoaa The historical accumulation function is added with the new commit and saved to DiagState. I think it is better to be saved to restart files if we have that functionality created in the future. The time step is needed to add the historical effect. I am using the MetState TSTEP for now and not sure if a TSTEP is also necessary for EmisState. Another thing I noticed is the temperature historical effect is deactivated by Silva but the historical PAR is still used.

@bbakernoaa
Copy link
Collaborator

@lwcugb I think it will but remember that CATChem is a column model and that the interface will pass that to the host model to be written out. It is also something that could be read in through the host model as well.

TSTEP would be the correct variable.

We could add an option flag to turn the effects on or off. Do you think that would be beneficial?

@lwcugb
Copy link
Author

lwcugb commented Jan 23, 2025

@lwcugb I think it will but remember that CATChem is a column model and that the interface will pass that to the host model to be written out. It is also something that could be read in through the host model as well.

TSTEP would be the correct variable.

We could add an option flag to turn the effects on or off. Do you think that would be beneficial?

Silva's paper suggests that the emissions are only slightly lower. Maybe his other edits make up for the historical temperature or the impact itself is small in the default algorithm (historical PAR may be more important?). So I guess it may not benefit a lot if we keep Silva's method.

@bbakernoaa bbakernoaa self-requested a review January 27, 2025 16:02
@bbakernoaa bbakernoaa requested a review from zmoon January 29, 2025 17:40
Copy link
Collaborator

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

I've made some comments regarding the config. Some of the other processes have similar issues, but we don't need to worry about that in this PR.

@lwcugb lwcugb requested a review from zmoon January 30, 2025 01:25
@bbakernoaa bbakernoaa requested a review from quaz115 February 3, 2025 17:19
Copy link
Collaborator

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

Added a few comments, looking good though.

Comment on lines 1443 to 1444
CALL CC_Error( errMsg, RC, thisLoc )
v_int = 1 ! default is one
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you want to allow using this default, you should use CC_Warning instead here (and the similar places below), since the rc is checked in Read_Input_File and it looks like it would cause early exit. Did you try it?

Copy link
Author

Choose a reason for hiding this comment

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

It won't crash the test since I did not use RETURN following CC_Error. I changed it to CC_Warning anyways cause it is not an error per se. I did a few tests and the real issue is QFYAML_Add_Get seems to always be successful although I commented out the scheme_opt line in the configuration file. So, the default value will not be assigned and it will go with the missing value. We probably need to revisit the qfyaml_mod. @bbakernoaa

@zmoon
Copy link
Collaborator

zmoon commented Apr 3, 2025

CMake Error at build/_deps/yaml-cpp-src/CMakeLists.txt:2 (cmake_minimum_required):
  Compatibility with CMake < 3.5 has been removed from CMake.

  Update the VERSION argument <min> value.  Or, use the <min>...<max> syntax
  to tell CMake that the project requires at least <min> but has been updated
  to work with policies introduced by <max> or earlier.

  Or, add -DCMAKE_POLICY_VERSION_MINIMUM=3.5 to try configuring anyway.

This is a problem with one of the MUSICA/MICM deps that is automatically pulled in. @lwcugb you could try adding -DCMAKE_POLICY_VERSION_MINIMUM=3.5 to the configure command in CI to fix it for now.

@bbakernoaa bbakernoaa self-requested a review April 7, 2025 19:20
@bbakernoaa bbakernoaa added enhancement New feature or request New Process Used for new processes labels Apr 14, 2025
@bbakernoaa bbakernoaa moved this to In Progress in CATChem Development Apr 14, 2025
Copy link
Collaborator

@quaz115 quaz115 left a comment

Choose a reason for hiding this comment

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

Some questions on the diagnostics, handling cold run and restart runs and, time step integration (if not same for emissions and MET)

bbakernoaa pushed a commit to bbakernoaa/CATChem that referenced this pull request Jun 13, 2025
Initial commit of historical leaf temp/par averaging approach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request New Process Used for new processes

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants