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

Switch to shr_drydep_mod module #1843

Merged
merged 3 commits into from
Sep 7, 2022
Merged

Conversation

fvitt
Copy link
Contributor

@fvitt fvitt commented Sep 2, 2022

Description of changes

Use shr_drydep_mod rather than the seq_drydep_mod module (which is merely a wrapper to shr_drydep_mod).

Specific notes

Namelist option drydep_method is removed. The other dry deposition options were obsolete and were removed from the code base. The only method available now is the "land" method. We are dependent on the land model to provide deposition velocities of chemical constituents over land in all configurations.

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #): #1822

Are answers expected to change (and if so in what way)? No
Expect bit-for-bit unchanged

Any User Interface Changes (namelist or namelist defaults changes)?
Namelist option drydep_method is removed

Testing performed, if any:
Only tested a few F compsets with chemistry

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability tag: enh - new science labels Sep 2, 2022
@ekluzek ekluzek marked this pull request as ready for review September 2, 2022 16:15
@ekluzek
Copy link
Collaborator

ekluzek commented Sep 2, 2022

@fvitt thanks so much for putting this together, and so quickly! That's really helpful. We need to figure out when and how to bring this in, but having a PR makes it possible to do that.

I did a quick look at your changes and they seem straightforward. But, I might get back to you when we take a closer look.

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 2, 2022

@fvitt it looks like one change is to assume that drydep is going to be calculated inside of CTSM, rather than the option to switch it between CAM or CTSM or a table. So this removes drydep_method and lnd_drydep variables. I just wanted to make sure I understood that change.

@ekluzek ekluzek self-requested a review September 2, 2022 16:21
@fvitt
Copy link
Contributor Author

fvitt commented Sep 2, 2022

@fvitt it looks like one change is to assume that drydep is going to be calculated inside of CTSM, rather than the option to switch it between CAM or CTSM or a table. So this removes drydep_method and lnd_drydep variables. I just wanted to make sure I understood that change.

Yes, that is correct.

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

Looks great. I like the idea that this simplifies the code to make it always assume that dry-dep is coming from CTSM.

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 2, 2022

@fvitt just FYI @adrifoster is going to take on bringing this PR to main-dev CTSM. It's now scheduled as the next tag. She is going to add another externals update and do more testing with it and then bring it in as the next CTSM tag. After that she has the ozone work that builds on top of this.

@adrifoster
Copy link
Collaborator

Thanks @fvitt for doing this PR so quickly!!

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 4, 2022

@fvitt @adrifoster ran into some unexpected fails as follows. Can you look into this and provide a fix for it?

FAIL ERI_Ld9.f10_f10_mg37.I2000Clm50BgcCru.cheyenne_gnu.clm-drydepnomegan RUN time=32
FAIL ERP_D_Ld5.f10_f10_mg37.I1850Clm50Bgc.cheyenne_gnu.clm-drydepnomegan RUN time=20
FAIL ERP_D_Ld5.f10_f10_mg37.IHistClm50SpCru.cheyenne_gnu.clm-drydepnomegan RUN time=35
FAIL SMS_D.1x1_brazil.I2000Clm51FatesSpCruRsGs.cheyenne_gnu.clm-FatesColdDefDryDepReducedComplexSatPhen RUN time=195
FAIL SMS_D_Ln9_P36x3.f19_g17.IHistClm50Sp.cheyenne_intel.clm-waccmx_offline RUN time=71
FAIL SMS_D_Ln9_P36x3_Vmct.f19_g17.IHistClm50Sp.cheyenne_intel.clm-waccmx_offline RUN time=54

From cesm log of one of them:

8:At line 169 of file /glade/work/afoster/ctsm_fates/src/biogeochem/DryDepVelocity.F90
8:Fortran runtime error: Index '1' of dimension 1 of array 'mapping' above upper bound of 0

        modified:   bld/CLMBuildNamelist.pm
        modified:   bld/namelist_files/namelist_defaults_drydep.xml
        modified:   bld/namelist_files/namelist_definition_drv_flds.xml
        modified:   src/main/controlMod.F90
@fvitt
Copy link
Contributor Author

fvitt commented Sep 6, 2022

@ekluzek
I have pushed updates to my branch which should fix the I compset test issues. There is a check in controlMod that does not make since to me when FATES is turned on. It seems to me that code block could be removed.

@ekluzek
Copy link
Collaborator

ekluzek commented Sep 6, 2022

@fvitt yes thanks for sending fixes in. @adrifoster will try it out and let us know if this resolves everything.

Yes, you are correct that check in controlMod wasn't correctly constructed, and @adrifoster will just remove it completely.

@adrifoster
Copy link
Collaborator

@fvitt @ekluzek the tests with this latest push passed! Thanks @fvitt!

@adrifoster
Copy link
Collaborator

From #270, we are doing testing to bring CTSM externals up to date now:

aux_clm tests for CTSM with cdeps0.12.42 - we expect lots of answer changes, a couple run fails (on Izumi) that will get fixed later

Cheyenne aux_clm: /glade/scratch/afoster/tests_0831-145952c - all DIFF fails and expected RUN fails

Izumi aux_xlm: /scratch/cluster/afoster/tests_0831-144810iz - all DIFF fails, RUN fails that will get fixed in a subsequent externals update:

    FAIL ERP_D_Ld5.f10_f10_mg37.I1850Clm50Bgc.izumi_intel.clm-nlevgrnd_small RUN time=149
    PEND ERP_D_Ld5.f10_f10_mg37.I1850Clm50Bgc.izumi_intel.clm-nlevgrnd_small COMPARE_base_rest
    FAIL ERS_Ld30.f45_f45_mg37.I2000Clm50FatesRs.izumi_intel.clm FatesColdDefReducedComplexFixedBiogeo RUN time=91

aux_clm tests for CTSM with cdeps0.12.62 - we expect some answer changes for specific tests (NEON and UMBS)

Cheyenne aux_clm: **/glade/scratch/afoster/tests_0831-175106ch ** - mostly NLCOMP fails, DIFF fails for single-point/NEON (as expected), other DIFF fails that are expected based on a cdeps NLDAS update, CREATE_NEWCASE fail that is now an existing issue:

    FAIL ERS_D_Ld5_Mmpi-serial.1x1_mexicocityMEX.I1PtClm50SpRs.cheyenne_gnu.clm-CLM1PTStartDate BASELINE ctsm5.1.dev106-cdeps42: DIFF
    FAIL SMS_Ld12_Mmpi-serial.1x1_vancouverCAN.I1PtClm50SpRs.cheyenne_gnu.clm-output_sp_highfreq BASELINE ctsm5.1.dev106-cdeps42: DIFF
    FAIL SMS_Ld1_PS.nldas2_rnldas2_mnldas2.I2000Ctsm50NwpSpNldas.cheyenne_gnu.clm-default BASELINE ctsm5.1.dev106-cdeps42: DIFF
    FAIL SMS_Ld1_PS.nldas2_rnldas2_mnldas2.I2000Ctsm50NwpSpNldasRs.cheyenne_gnu.clm-default BASELINE ctsm5.1.dev106-cdeps42: DIFF
    FAIL SMS_D_Lm1_Mmpi-serial.CLM_USRDAT.I1PtClm50SpRs.cheyenne_intel.clm-USUMB_nuopc BASELINE ctsm5.1.dev106-cdeps42: DIFF
 FAIL SMS_D_Ld1_PS.f09_g17.I1850Clm50BgcSpinup.cheyenne_intel.clm-cplhist CREATE_NEWCASE

Izumi aux_xlm: /scratch/cluster/afoster/tests_0831-175502iz - mostly NLCOMP fails, DIFF fails for single point as expected

    FAIL ERS_D_Ld5_Mmpi-serial.1x1_vancouverCAN.I1PtClm50SpRs.izumi_nag.clm-CLM1PTStartDate BASELINE ctsm5.1.dev106-cdeps42: DIFF
    FAIL SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-default--clm-NEON-NIWO BASELINE ctsm5.1.dev106-cdeps42: DIFF

aux_clm tests for Externals updated to cmeps0.13.71, cpl7.0.14, and cdeps0.12.63 with this branch's (plus my updates) updates:

Cheyenne aux_clm: **/glade/scratch/afoster/tests_0906-141452ch ** - all NLCOMP and FIELDLIST diffs

Izumi aux_clm: /scratch/cluster/afoster/tests_0906-141620iz - all NLCOMP and FIELDLIST diffs

@billsacks and @ekluzek anything else that needs to get tested?

@billsacks
Copy link
Member

Great @adrifoster ! Thank you for doing all this testing! This seems good to me at this point.

@billsacks billsacks merged commit 1355650 into ESCOMP:master Sep 7, 2022
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability science Enhancement to or bug impacting science
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants