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

Connect ozone from atmosphere to CTSM #1837

Merged
merged 17 commits into from
Sep 8, 2022
Merged

Conversation

adrifoster
Copy link
Collaborator

@adrifoster adrifoster commented Aug 29, 2022

Description of changes

Allow DATM input of ozone partial pressure (monthly, mol/mol) to CTSM and use this ozone in the OzoneMod module.

Specific notes

Currently we do not downscale input ozone to sub-daily values, but this will be completed in a subsequent PR.

Contributors other than yourself, if any: @billsacks @mvertens

partially addresses #270

Are answers expected to change (and if so in what way)? Yes - ozone uptake and impact on photosynthesis will change because the ozone partial pressure now varies in time and space (rather than a static input parameter).

Any User Interface Changes (namelist or namelist defaults changes)? Yes - new ozone streams created (DATM_O3 can be clim_2000, clim_2010, clim_1850, hist, SSP2-4.5, SSP3-7.0, or SSP5-8.5).

Testing performed, if any:

General Testing

Performed tests to ensure the input ozone files matched reasonably well with output O3 from CTSM (see testing script /glade/u/home/afoster/analysis_scripts/Ozone_damage.ipynb). Example plots (global input/output difference at one time slice and then both input and output over one year at a single latitude and longitude)

bokeh_plot

bokeh_plot (1)

To check that these changes did indeed impact the ozone functioning - global plot of ozone coefficient for sunlit leaves for one time point (for master branch), and year-long plot of same variable for master and updated branch.

bokeh_plot (2)

bokeh_plot (3)

System Testing

  • aux_clm on Cheyenne (/glade/scratch/afoster/tests_0829-082121ch ) and izumi (/scratch/cluster/afoster/tests_0829-094849iz ). One on izumi includes an ozone test (ERP_D_Ld5_P48x1)

Will report back after tests finish

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

This basically looks great. Thanks so much for your work on it! I have just a few minor comments below.

@@ -86,7 +86,7 @@ module OzoneMod

! TODO(wjs, 2014-09-29) This parameter will eventually become a spatially-varying
! value, obtained from ATM
real(r8), parameter :: forc_ozone = 100._r8 * 1.e-9_r8 ! ozone partial pressure [mol/mol]
!real(r8), parameter :: forc_ozone = 100._r8 * 1.e-9_r8 ! ozone partial pressure [mol/mol]
Copy link
Member

@billsacks billsacks Aug 29, 2022

Choose a reason for hiding this comment

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

  • Please go ahead and remove this commented-out line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do! Do you want me to also remove your comment/note below about making some of these parameters input parameters? (here) Or leave for now?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I guess I'd leave that for now (though I don't feel strongly).

Externals.cfg Outdated
Comment on lines 51 to 58
tag = cmeps0.13.70-3-g2a3f929f8
protocol = git
repo_url = https://github.com/ESCOMP/CMEPS.git
repo_url = https://github.com/adrifoster/CMEPS.git
local_path = components/cmeps
required = True

[cdeps]
tag = cdeps0.12.41
tag = cdeps0.12.60-24-g7f47d60
Copy link
Member

@billsacks billsacks Aug 29, 2022

Choose a reason for hiding this comment

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

  • As discussed, before making the final tag, these should be updated to point to tags on master / main for both cmeps and cdeps. (Just making note of it here to remind us.)

Comment on lines 558 to 560
call hist_addfld1d (fname='ATM_O3', units='mol/mol', &
avgflag='A', long_name='atmospheric ozone partial pressure', &
ptr_lnd=this%forc_o3_grc)
Copy link
Member

@billsacks billsacks Aug 29, 2022

Choose a reason for hiding this comment

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

  • I think this should be inactive by default but then added to the ozone testmod(s)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just added this to the o3 test def variable list

@ekluzek
Copy link
Collaborator

ekluzek commented Aug 29, 2022

I suggest reverting the whitespace changes in CanopyFluxes, since the actual changes are small, and the whitespace is large there, and because CanopyFluxes changes more often than the Ozone files.

@adrifoster adrifoster changed the title Connect DATM ozone to CTSM Connect ozone from atmosphere to CTSM Aug 30, 2022
@adrifoster
Copy link
Collaborator Author

I suggest reverting the whitespace changes in CanopyFluxes, since the actual changes are small, and the whitespace is large there, and because CanopyFluxes changes more often than the Ozone files.

Just did this - looks like it worked well, I only see that one line change now in the diffs even with whitespace visible.

@adrifoster
Copy link
Collaborator Author

After chatting with @ekluzek and @billsacks we are going to do some testing in a series because CTSM is a bit behind in terms of the cdeps tags. These series of tests will ensure our answer changes are what we expect.

  1. 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
    - compare to ctsm5.1.dev106, generate an intermediate baseline ctsm5.1.dev106-cdeps42
  2. aux_clm tests for CTSM with cdeps0.12.62 - we expect some answer changes for specific tests (NEON and UMBS)
    - compare to intermediate baseline ctsm5.1.dev106-cdeps42, generate second intermediate baseline ctsm5.1.dev106-cdeps62
  3. aux_clm tests for CTSM with all (CTSM, CDEPS, CMEPS) ozone changes - should be minimal answer changes, only for ozone tests
    • compare to intermediate baseline ctsm5.1.dev106-cdeps62, generate ctsm5.1.dev107

@ekluzek and @billsacks does this seem right?

@ekluzek
Copy link
Collaborator

ekluzek commented Aug 31, 2022

@adrifoster yes that looks correct to me.

@billsacks
Copy link
Member

Yes, looks good @adrifoster - thank you for clearly documenting the plan!

@adrifoster
Copy link
Collaborator Author

Some other conflicts with CDEPS/CMEPS have come up and we are going to bring the Externals up to master for CDEPS, CMEPS, and cpl7 first before merging this PR in. We will also bring in #1843 with this externals update. The intermediate testing planned for this PR will still facilitate the externals update.

THEN we will bring in this tag.

@billsacks
Copy link
Member

Thanks for the latest change @adrifoster . I just realized that I had a pending review that I never submitted. The main thing I was asking there was to add this new o3 output to the other ozone test mod, which you have just done - thank you. The one other thing I was suggesting is that, in both o3 test mods, you add this new output to hist_fincl1 (grid cell output); then there is no need to also add it to hist_fincl2 (vector output). If you have already done testing, though, then I wouldn't worry about that change.

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

See the comment I just made about a possible change to the testmods. But that's optional, and basically this looks great, so marking it as approved - thank you!

@adrifoster
Copy link
Collaborator Author

Testing for this PR:

cheyenne aux_clm: /glade/scratch/afoster/tests_0907-160138ch - all FIELDLIST diffs (from cplhist) except for NLCOMP and BASELINE DIFF for the O3 test (which is good!)

ERP_P72x2_Ly3.f10_f10_mg37.I2000Clm50BgcCrop.cheyenne_intel.clm-irrig_o3falk_reduceOutput

izumi aux_clm: /scratch/cluster/afoster/tests_0907-201407iz - all FIELDLIST diffs (from cplhist) except for NLCOMP and BASELINE DIFF for O3 test (also good)

ERP_D_Ld5_P48x1.f10_f10_mg37.I2000Clm50Sp.izumi_nag.clm-o3lombardozzi2015

@adrifoster
Copy link
Collaborator Author

See the comment I just made about a possible change to the testmods. But that's optional, and basically this looks great, so marking it as approved - thank you!

I updated that test def for the falk test and re-did the test!

@billsacks billsacks merged commit f8e04aa into ESCOMP:master Sep 8, 2022
@adrifoster adrifoster deleted the ozone_update branch September 8, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to eat (Done!)
Development

Successfully merging this pull request may close these issues.

3 participants