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

cam6_3_071: Updates to deposition module in share code externals #602

Merged
merged 19 commits into from
Aug 3, 2022

Conversation

fvitt
Copy link

@fvitt fvitt commented Jun 1, 2022

These are updates needed to read deposition data from file. Changes to drydep_mod share code replaced hard-wired data in drydep_mod with a netcdf input file. Changes here are needed for the updates to the relevant external share code bases.

closes #601
closes #625

fvitt added 6 commits May 24, 2022 16:42
        modified:   bld/build-namelist
        modified:   bld/namelist_files/namelist_defaults_cam.xml
        modified:   bld/namelist_files/namelist_definition.xml
        modified:   src/chemistry/mozart/mo_neu_wetdep.F90
        modified:   src/cpl/mct/cam_cpl_indices.F90
        modified:   Externals.cfg
        modified:   src/chemistry/mozart/mo_chemini.F90
        modified:   src/chemistry/mozart/mo_drydep.F90
        modified:   src/chemistry/mozart/mo_neu_wetdep.F90
        modified:   src/control/camsrfexch.F90
        modified:   src/cpl/mct/atm_import_export.F90
        modified:   src/cpl/nuopc/atm_comp_nuopc.F90
        modified:   src/cpl/nuopc/atm_import_export.F90
Externals.cfg Outdated Show resolved Hide resolved
bld/namelist_files/namelist_defaults_cam.xml Show resolved Hide resolved
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

Hit approve button by accident - it should be request changes

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

I second @cacraigucar's requests and added a few more minor requests.

@@ -330,7 +329,7 @@ subroutine neu_wetdep_tend(lchnk,ncol,mmr,pmid,pdel,zint,tfld,delt, &
end do
!
! compute effective Henry's law coefficients
! code taken from models/drv/shr/seq_drydep_mod.F90
! code taken from models/drv/shr/shr_drydep_mod.F90
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file does not exist. Did you mean

Suggested change
! code taken from models/drv/shr/shr_drydep_mod.F90
! code taken from components/cmeps/cesm/nuopc_cap_share/shr_drydep_mod.F90

or

Suggested change
! code taken from models/drv/shr/shr_drydep_mod.F90
! code taken from components/cpl7/driver/shr/shr_drydep_mod.F90

?? Something else??

Copy link
Author

Choose a reason for hiding this comment

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

Removed the comment

@@ -27,6 +27,7 @@ module atm_import_export
implicit none
private ! except

public :: set_options
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a good name for the cap level. Please change it to

Suggested change
public :: set_options
public :: read_surface_namelists

or come up with some other appropriate (i.e., more specific) name.

Copy link
Author

Choose a reason for hiding this comment

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

Would you be okay with read_fields_namelists?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would read_surface_field_namelists be more specific while still being accurate?

Copy link
Author

Choose a reason for hiding this comment

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

Updated as suggested

@@ -66,14 +67,25 @@ module atm_import_export
contains
!===============================================================================

subroutine advertise_fields(gcomp, flds_scalar_name, rc)
subroutine set_options()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change name (see above).

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -29,7 +29,7 @@ module atm_comp_nuopc
use cam_logfile , only : iulog
use spmd_utils , only : spmdinit, masterproc, iam, mpicom
use time_manager , only : get_curr_calday, advance_timestep, get_curr_date, get_nstep, get_step_size
use atm_import_export , only : advertise_fields, realize_fields
use atm_import_export , only : set_options, advertise_fields, realize_fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change name (see comment in atm_import_export.F90).

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines 281 to 282
! read drv flds options
call set_options()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change name (see comment in atm_import_export.F90).

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -278,6 +278,9 @@ subroutine InitializeAdvertise(gcomp, importState, exportState, clock, rc)
call shr_sys_abort(subname//'Need to set attribute ScalarFieldIdxNextSwCday')
endif

! read drv flds options
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change the comment as this has nothing to do with the driver:

Suggested change
! read drv flds options
! read mediator fields options

Copy link
Author

Choose a reason for hiding this comment

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

updated the comment

        modified:   src/chemistry/mozart/mo_neu_wetdep.F90
        modified:   src/cpl/nuopc/atm_comp_nuopc.F90
        modified:   src/cpl/nuopc/atm_import_export.F90
… comments

        modified:   src/cpl/nuopc/atm_comp_nuopc.F90
        modified:   src/cpl/nuopc/atm_import_export.F90
        modified:   src/ionosphere/waccmx/edyn_init.F90
… set ROF_NCPL for weimer test

        modified:   cime_config/config_pes.xml
        modified:   cime_config/testdefs/testlist_cam.xml
        modified:   cime_config/testdefs/testmods_dirs/cam/waccmx_weimer/shell_commands
jedwards4b added a commit to ESCOMP/CMEPS that referenced this pull request Jul 26, 2022
Description of changes

This moves the hard-wired data from seq_drydep_mod to an input file that can be easily replaced to provide flexibility.
Specific notes

This PR needs to be coordinated with
ESCOMP/CAM#602

See relevant discussion in:
ESCOMP/CESM_share#8

Contributors other than yourself, if any:

CMEPS Issues Fixed (include github issue #):
ESCOMP/CESM_share#8

Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial)
bfb

Any User Interface Changes (namelist or namelist defaults changes)?
Added dep_data_file (in drv_flds_in) for the path of the input data file containing Henrys Law coefficients
Testing performed

Testing performed if application target is CESM:

(recommended) CIME_DRIVER=nuopc scripts_regression_tests.py

    machines: cheyenne, izumi
    details (e.g. failed tests):

(recommended) CESM testlist_drv.xml

    machines and compilers:
    details (e.g. failed tests):

(optional) CESM prealpha test

    machines and compilers
    details (e.g. failed tests):

    aux_cam tests
        machines and compilers: cheyenne-intel, izumi-gnu, izumi-nag
        details (e.g. failed tests):

Testing performed if application target is UFS-coupled:

    (recommended) UFS-coupled testing
        description:
        details (e.g. failed tests):

Testing performed if application target is UFS-HAFS:

    (recommended) UFS-HAFS testing
        description:
        details (e.g. failed tests):

Hashes used for testing:

CESM:

    repository to check out: ESCOMP/CESM.git
    branch/hash:

UFS-coupled, then umbrella repostiory to check out and associated hash:

    repository to check out:
    branch/hash:

UFS-HAFS, then umbrella repostiory to check out and associated hash:

    repository to check out:
    branch/hash:
jedwards4b added a commit to ESCOMP/CESM_CPL7andDataComps that referenced this pull request Jul 26, 2022
Read Henrys Law coefficients from file


This moves the hard-wired data from seq_drydep_mod to an input file that can be easily replaced to provide flexibility.

This PR needs to be coordinated with
ESCOMP/CAM#602

See relevant discussion in:
ESCOMP/CESM_share#8
        modified:   Externals.cfg
@fvitt fvitt requested a review from gold2718 July 27, 2022 13:13
@lizziel
Copy link
Collaborator

lizziel commented Jul 27, 2022

I am using the updates in this PR (and the related ones) for development of GEOS-Chem in CESM. One of the things that has confused me is that wet deposition requires parameters from dry deposition modules, previously for seq_drydep_mod, and now for shr_drydep_mod. I see the netcdf file that replaces the hard-coded values in seq_drydep_mod.F90 is more generically named to indicate deposition rather than dry deposition (dep_data_file.nc rather than drydep_data_file.nc). Would you consider updating the module names to be generic as well? I can add this comment to the conversations of the relevant PRs if you agree.

@fvitt
Copy link
Author

fvitt commented Jul 27, 2022

@lizziel Your request is a little late for this PR. The PRs for the externals have been merged into the main repos and have been closed. This PR is close to being merged into cam_development as it is. I suggest you open a new issue in https://github.com/ESCOMP/CMEPS for your request.

@lizziel
Copy link
Collaborator

lizziel commented Jul 27, 2022

@fvitt, thanks for the response. I'll open a new issue for this.

Copy link
Collaborator

@nusbaume nusbaume 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! Just a couple optional cosmetic changes (and a question for the other CAM SEs).

@@ -18,7 +18,7 @@ module mo_drydep
use dyn_grid, only : get_dyn_grid_parm, get_horiz_grid_d
use scamMod, only : single_column

use seq_drydep_mod, only : nddvels => n_drydep, drydep_list, mapping
use shr_drydep_mod, only : nddvels => n_drydep, drydep_list, mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might remove the spaces between only and : here in order to better match the CAM style guide.

@@ -12,7 +12,7 @@ module mo_neu_wetdep
use constituents, only : pcnst
use spmd_utils, only : masterproc
use cam_abortutils, only : endrun
use seq_drydep_mod, only : n_species_table, species_name_table, dheff
use shr_drydep_mod, only : n_species_table, species_name_table, dheff
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might remove the spaces between only and : here in order to better match the CAM style guide.

real(r8) :: lats(pcols)

real(r8), parameter :: rad2deg = 180._r8/pi
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gold2718 @cacraigucar @peverwhee I feel like I have seen this particular calculation in multiple places throughout CAM. Would it make sense to add this (and an inverse deg2rad) into physconst so that this parameter doesn't have to be repeated so much?

I should note that we don't have to necessarily do it for this PR, I just think that if we all agree then it would be good to make an issue for it so one of us can tackle it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's minor in the sense that the parameter is baked into the code (no run-time cost).
A better solution for cases like this will come when all the dycores use the new physics grid infrastructure which has coordinates in both radians and degrees.

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Looks okay to me

@cacraigucar cacraigucar changed the title Updates to deposition module in share code externals part of cam6_3_071: Updates to deposition module in share code externals Aug 1, 2022
@cacraigucar cacraigucar changed the title part of cam6_3_071: Updates to deposition module in share code externals cam6_3_071: Updates to deposition module in share code externals Aug 1, 2022
@fvitt fvitt merged commit 7d7b2e6 into ESCOMP:cam_development Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB bit for bit tag
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

6 participants