Reduce duplication between caps #1086

slevis-lmwg · 2020-07-27T22:13:43Z

Description of changes

The mct, nuopc and lilac caps duplicate some code as documented in #918
and this PR attempts to reduce such duplication.

Specific notes

Contributors other than yourself, if any:
@billsacks

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

Are answers expected to change (and if so in what way)?
Theoretically no if we functionalize the exact code that repeats between caps; however, the repeating calculation of qsat can instead be replaced with a call to already existing subroutine QSat. I have now confirmed round-off rms diffs (< 1e-17 in my test's cprnc.out file) when making subr. QSat identical to the repeating caps calculation. On the other hand, my qsat values change by more than 1e-2 when I call the model's unchanged QSat algorithm. Despite these larger diffs, I recommend this solution to enhance consistency throughout the CTSM.

Any User Interface Changes (namelist or namelist defaults changes)?
No.

Testing performed, if any:
At the time that I created this PR, I had completed this test:
./create_test ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc -c /glade/p/cgd/tss/ctsm_baselines/ctsm1.0.dev105
which passed except in comparison to ctsm_baselines as explained above.

slevis-lmwg · 2020-07-27T22:34:38Z

src/cpl/mct/lnd_import_export.F90

-       qsat           = 0.622_r8*e / (forc_pbot - 0.378_r8*e)
+       qsat_old       = 0.622_r8*e / (forc_pbot - 0.378_r8*e)
+       call QSat_temp(forc_t, forc_pbot, dum1, dum2, qsat_kg_kg, dum3)
+       if (qsat_kg_kg - qsat_old > 1.e-14_r8) write(iulog,*) 'qsat_new, qsat_old =', qsat_kg_kg, qsat_old  ! slevis diag


In the subr. call above, QSat_temp returns round-off diffs, while QSat returns diffs that exceed round-off.

I propose that we call QSat for consistency throughout the CTSM, despite the larger diffs.

@billsacks I will request a code review of my changes this far, in order to initiate a discussion on whether this solution is acceptable.

Once this piece is resolved, we can discuss the functionalizing of remaining sections of repeating code in the caps.

This has two benefits: (1) It prevents callers from needing to create dummy variables to hold outputs that they don't actually need. (2) If neither qsdT nor esdT are needed, then we can avoid doing some expensive calculations.

billsacks · 2020-07-31T23:32:54Z

@slevisconsulting sorry for my slow reply here. Yes, you raise a very good point – that we should probably be using the existing QSat routine consistently. From a quick look, it looks like the existing QSat routine uses a higher-order polynomial approximation, explaining the greater-than-roundoff-level diffs you were seeing. Is that your understanding, too? (I wanted to make sure that these greater-than-roundoff-level diffs are expected, and they do seem to be.) Thanks for doing the careful checking you have done so far.

Your changes here inspired me to make the changes in #1094 - here and in some other places, not all of the output arguments from QSat are needed. It feels to me like the changes in #1094 lead to improved code clarity (avoiding the need for temporary, unused variables) and possibly some performance improvements (avoiding some expensive calculations of the derivatives if they aren't needed). Do you want to take a quick look at that and let me know if that looks like a good path forward? If so, it seems like the approach would be:

I bring Make most QSat output arguments optional #1094 to master (bfb)
You make the minimal changes to use the existing QSat (answer changing)
Then you make the additional changes to remove duplication (should be bfb)

(I'm suggesting separating the bfb from answer-changing mods because the bfb mods are so extensive and I don't want accidental answer changes to get mixed in with them.)

Does that sound like a good plan? As always, things are more difficult than I originally imagined, but I think this will be helpful cleanup; do you agree?

slevis-lmwg · 2020-08-01T00:05:40Z

@billsacks
Yes to all your questions.

I will review #1094 by Monday.

billsacks · 2020-08-06T21:00:13Z

@slevisconsulting given our backlog of tags, I thought it would be best if I revised my above plan so that this only results in a single tag, rather than 3. But I think it's still best if the full test suite is run separately on each stage, to verify that changes that are expected to be bfb are indeed bfb.

I just ran the full test suite on #1094 and it was bit-for-bit with master. So I'd suggest that you merge that branch into yours... but you may want to first back out some of your temporary changes in QSatMod to avoid merge conflicts, then you can reapply them later. Then you can move forward with development of this branch.

…ce_duplication

slevis-lmwg · 2020-08-07T00:28:02Z

the approach would be:

I bring Make most QSat output arguments optional #1094 to master (bfb)

You make the minimal changes to use the existing QSat (answer changing)

Then you make the additional changes to remove duplication (should be bfb)

(I'm suggesting separating the bfb from answer-changing mods because the bfb mods are so extensive and I don't want accidental answer changes to get mixed in with them.)

@billsacks I wanted to be clear about the next steps:

I have completed the modified step (1)
(a) I ran my single test (same as above) and confirmed round-off diffs when calling QSat_temp as reported before
(b) So based on your suggestion, I should proceed to running the full test suite with all my mods but without using QSat_temp, and I should generate a temporary baseline
Additional changes (should be bfb)

billsacks · 2020-08-07T12:26:37Z

@billsacks I wanted to be clear about the next steps:

I have completed the modified step (1)

(a) I ran my single test (same as above) and confirmed round-off diffs when calling QSat_temp as reported before
(b) So based on your suggestion, I should proceed to running the full test suite with all my mods but without using QSat_temp, and I should generate a temporary baseline

Additional changes (should be bfb)

Yes, that is my thinking if it makes sense to you.

slevis-lmwg · 2020-08-10T21:47:12Z

Testing confirmed on izumi and cheyenne:
Round-off diffs when calling QSat_temp from src/cpl/mct/lnd_import_export.F90

Testing confirmed on izumi and cheyenne:
Greater than round-off when calling QSat from src/cpl/mct/lnd_import_export.F90

Generated temp. baselines:
/fs/cgd/csm/ccsm_baselines/ctsm1.0.105_qsat
/glade/p/cgd/tss/ctsm_baselines/ctsm1.0.105_qsat

slevis-lmwg · 2020-08-13T00:45:22Z

Izumi test suite passed!

slevis-lmwg · 2020-08-14T17:38:40Z

Cheyenne test suite passed.

@billsacks this work is ready for review.

Questions regarding my implementation:

Placement of new file lnd_import_export_utils.F90 in /src/utils/. I tried placing in a new /src/cpl/utils but didn't know how to make the new file visible during the build.
Treatment of optional fields: Tempting to place those in the same new subroutine but also seemed a bit outside the scope of Some science-y processing of atmospheric imports duplicated between caps #918 and I wasn't sure if it was worth the additional effort right now.
Further modularization: Eg place the error checks in their own subroutine. Seems logical but not sure if worth the additional effort right now.

billsacks · 2020-08-21T23:09:03Z

Placement of new file lnd_import_export_utils.F90 in /src/utils/. I tried placing in a new /src/cpl/utils but didn't know how to make the new file visible during the build.

Putting this in src/cpl/utils is the right thing to do here. You should just need to add a line in cime_config/buildlib for this to work

Treatment of optional fields: Tempting to place those in the same new subroutine but also seemed a bit outside the scope of Some science-y processing of atmospheric imports duplicated between caps #918 and I wasn't sure if it was worth the additional effort right now.

I agree (not necessary right now)

Further modularization: Eg place the error checks in their own subroutine. Seems logical but not sure if worth the additional effort right now.

I also agree (not necessary right now)

billsacks

Thanks a lot for this! I have two optional requests inline; if you don't have time and/or don't agree with me on them, then they aren't critical. See also my comment from a few minutes ago in this PR.

Thanks for consolidating some of the error checking code in addition to the code that sets derived quantities!

billsacks · 2020-08-21T23:11:01Z

src/utils/lnd_import_export_utils.F90

+    !--------------------------
+    ! Error checks
+    !--------------------------
+
+    ! Check that solar, specific-humidity, and LW downward aren't negative
+    do g = begg, endg
+       if ( atm2lnd_inst%forc_lwrad_not_downscaled_grc(g) <= 0.0_r8 ) then
+          call shr_sys_abort( subname//&
+               ' ERROR: Longwave down sent from the atmosphere model is negative or zero' )
+       end if
+       if ( (atm2lnd_inst%forc_solad_grc(g,1) < 0.0_r8) .or. &
+            (atm2lnd_inst%forc_solad_grc(g,2) < 0.0_r8) .or. &
+            (atm2lnd_inst%forc_solai_grc(g,1) < 0.0_r8) .or. &
+            (atm2lnd_inst%forc_solai_grc(g,2) < 0.0_r8) ) then
+          call shr_sys_abort( subname//&
+               ' ERROR: One of the solar fields (indirect/diffuse, vis or near-IR)'// &
+               ' from the atmosphere model is negative or zero' )
+       end if
+       if ( wateratm2lndbulk_inst%forc_q_not_downscaled_grc(g) < 0.0_r8 )then
+          call shr_sys_abort( subname//&
+               ' ERROR: Bottom layer specific humidty sent from the atmosphere model is less than zero' )
+       end if
+    end do
+
+    ! Make sure relative humidity is properly bounded
+    ! atm2lnd_inst%forc_rh_grc(g) = min( 100.0_r8, atm2lnd_inst%forc_rh_grc(g) )
+    ! atm2lnd_inst%forc_rh_grc(g) = max(   0.0_r8, atm2lnd_inst%forc_rh_grc(g) )


Not critical, but:

[optional] it would be best if this error check code was put in its own subroutine, since it is pretty unrelated to the rest of the subroutine, and the name of the subroutine doesn't give any indication that it also is doing this error checking.

I made the change and submitted this test:
./create_test ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc -c /glade/p/cgd/tss/ctsm_baselines/ctsm1.0.105_qsat PASS

Question:
Ok that I replaced the corresponding code with a call to this new subroutine? Or would you write the new call three times, once after each call derive_quantities?

Thanks @slevisconsulting ! I was imagining writing the new call three times, once after each call to derive_quantities. I know that means a bit of duplication, but the benefit is that, when you're reading through the top-level routine, it becomes more clear what's happening. As it's written now, when you read through the top-level routine, there's no indication that there is any error checking being done, unless you really dig deep into the implementation. The alternative would be to rename 'derive_quantities' to 'derive_quantities_and_check_for_errors', but my general feeling is that if you have a subroutine name that implies two separate activities, then it deserves to be split into its two pieces which are called in succession from the top level.

So, if you agree, can you please move the call to check_for_errors to the 3 top-level routines?

I made the change and submitted this test again:
./create_test ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc -c /glade/p/cgd/tss/ctsm_baselines/ctsm1.0.105_qsat PASS

billsacks · 2020-08-21T23:17:23Z

src/main/atm2lndType.F90

+     real(r8), pointer :: forc_rainc_grc                (:)   => null() ! convective rain (mm/s)
+     real(r8), pointer :: forc_rainl_grc                (:)   => null() ! large scale rain (mm/s)
+     real(r8), pointer :: forc_snowc_grc                (:)   => null() ! convective snow (mm/s)
+     real(r8), pointer :: forc_snowl_grc                (:)   => null() ! large scale snow (mm/s)


Not critical, but:

[optional] I feel it would be best if we avoided adding these variables to atm2lndType. Since they are only needed in the scope of lnd_import, I think they can be local arrays in that routine (declared like forc_rainc(bounds%begg:bounds%endg), as in the prior version of the lilac and nuopc caps), then passed individually to your new shared subroutine. (i.e., the new subroutine would have 4 extra arguments, accepting these 4 arrays). The point of this would be to avoid bloating this derived type. But I don't feel strongly about this.

I made the change and submitted this test:
./create_test ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc -c /glade/p/cgd/tss/ctsm_baselines/ctsm1.0.105_qsat PASS

I forgot to remove these from atm2lndType, so doing that now.

slevis-lmwg · 2020-08-24T14:28:41Z

Putting this in src/cpl/utils is the right thing to do here. You should just need to add a line in cime_config/buildlib for this to work

I made the change and submitted this test:
./create_test ERS_D_Ld6.f10_f10_musgs.I1850Clm45BgcCrop.cheyenne_intel.clm-clm50CMIP6frc -c /glade/p/cgd/tss/ctsm_baselines/ctsm1.0.105_qsat PASS

...from within subroutine derive_quantities to after each call derive_quantities

billsacks

Looks great now - thank you!

Fang Li's latest Fire version - includes allowing clm5.1 phys version. New physics option is added called "clm5_1", with currently the new feature to use the latest fire changes. This has some adjustments to the fire model and includes some changes to the parameter file. Other new features will be added into clm5_1 in future tags. Also bring in mksurfdata changes for the raw urban dataset change. This adds some changes to mksurfdata for a new urban raw dataset, as well as preparation for new changes for some other urban changes that will be a future part of clm5_1. Also use the half degree lightning dataset by default for clm5_1. Start adding a new test list ctsm_sci that tests all the scientifically supported compsets. Some of those tests fail due to existing issues, that will be fixed later. Some more work done to change clm to ctsm, and allow for ctsm as a component.

slevis-lmwg · 2020-09-25T14:45:03Z

Test-suite on cheyenne: PASS

slevis-lmwg · 2020-09-25T15:36:52Z

Test-suite on izumi: PASS

slevis-lmwg · 2020-09-25T15:42:52Z

@billsacks this PR is ready for merge. Thanks.

billsacks · 2020-09-25T15:55:43Z

Thanks @slevisconsulting . FYI I made some very minor ChangeLog edits in 3ea45a6.

Functionalizing the calculation of qsat

b20b7d0

slevis-lmwg commented Jul 27, 2020

View reviewed changes

slevis-lmwg requested a review from billsacks July 27, 2020 22:39

billsacks added 3 commits July 31, 2020 15:33

Remove unused rhoSat subroutine

f057735

Fix indentation

357b93d

Make most QSat output arguments optional

a5a1731

This has two benefits: (1) It prevents callers from needing to create dummy variables to hold outputs that they don't actually need. (2) If neither qsdT nor esdT are needed, then we can avoid doing some expensive calculations.

billsacks mentioned this pull request Jul 31, 2020

Make most QSat output arguments optional #1094

Merged

Minor refactor suggested by Sam Levis

8dace41

Merge remote-tracking branch 'billsacks/qsat_optional_args' into redu…

e87add8

…ce_duplication

Remove all QSat_temp codes that I had added for testing reasons only

c1985c0

Consolidation of repetitive code with bit-for-bit same answers

766a2ea

slevis-lmwg added 2 commits August 14, 2020 11:41

Small corrections to allow entire test-suite to pass

2c6d0d9

First draft of my ChangeLog updates for this PR

53376e2

billsacks requested changes Aug 21, 2020

View reviewed changes

slevis-lmwg added 3 commits August 24, 2020 08:33

Moved lnd_import_export_utils.F90 from /src/utils to /src/cpl/utils

251a170

Revisions in response to @billsacks review

1b0c9b7

Minor clean-up

98151b5

Move the call check_for_errors

796514c

...from within subroutine derive_quantities to after each call derive_quantities

billsacks approved these changes Aug 26, 2020

View reviewed changes

Correction for LILACSMOKE test to pass; cheyenne test-suite now ok

003dd9e

Test-suite results reported in ChangeLog

14a16e8

billsacks merged commit f3e44c8 into ESCOMP:master Sep 25, 2020

glemieux mentioned this pull request Jan 26, 2021

Merge ctsm5.1.dev020 tag into fates_main_api #1257

Merged

slevis-lmwg deleted the reduce_duplication branch February 5, 2021 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce duplication between caps #1086

Reduce duplication between caps #1086

slevis-lmwg commented Jul 27, 2020

slevis-lmwg Jul 27, 2020

slevis-lmwg Jul 27, 2020

billsacks commented Jul 31, 2020

slevis-lmwg commented Aug 1, 2020

billsacks commented Aug 6, 2020

slevis-lmwg commented Aug 7, 2020 •

edited

Loading

billsacks commented Aug 7, 2020

slevis-lmwg commented Aug 10, 2020 •

edited

Loading

slevis-lmwg commented Aug 13, 2020

slevis-lmwg commented Aug 14, 2020

billsacks commented Aug 21, 2020 •

edited by slevis-lmwg

Loading

billsacks left a comment

billsacks Aug 21, 2020 •

edited by slevis-lmwg

Loading

slevis-lmwg Aug 24, 2020

billsacks Aug 24, 2020 •

edited

Loading

slevis-lmwg Aug 25, 2020

billsacks Aug 21, 2020 •

edited by slevis-lmwg

Loading

slevis-lmwg Aug 24, 2020 •

edited

Loading

slevis-lmwg Aug 24, 2020

slevis-lmwg commented Aug 24, 2020 •

edited

Loading

billsacks left a comment

slevis-lmwg commented Sep 25, 2020

slevis-lmwg commented Sep 25, 2020

slevis-lmwg commented Sep 25, 2020

billsacks commented Sep 25, 2020

Reduce duplication between caps #1086

Reduce duplication between caps #1086

Conversation

slevis-lmwg commented Jul 27, 2020

Description of changes

Specific notes

slevis-lmwg Jul 27, 2020

Choose a reason for hiding this comment

slevis-lmwg Jul 27, 2020

Choose a reason for hiding this comment

billsacks commented Jul 31, 2020

slevis-lmwg commented Aug 1, 2020

billsacks commented Aug 6, 2020

slevis-lmwg commented Aug 7, 2020 • edited Loading

billsacks commented Aug 7, 2020

slevis-lmwg commented Aug 10, 2020 • edited Loading

slevis-lmwg commented Aug 13, 2020

slevis-lmwg commented Aug 14, 2020

billsacks commented Aug 21, 2020 • edited by slevis-lmwg Loading

billsacks left a comment

Choose a reason for hiding this comment

billsacks Aug 21, 2020 • edited by slevis-lmwg Loading

Choose a reason for hiding this comment

slevis-lmwg Aug 24, 2020

Choose a reason for hiding this comment

billsacks Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

slevis-lmwg Aug 25, 2020

Choose a reason for hiding this comment

billsacks Aug 21, 2020 • edited by slevis-lmwg Loading

Choose a reason for hiding this comment

slevis-lmwg Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

slevis-lmwg Aug 24, 2020

Choose a reason for hiding this comment

slevis-lmwg commented Aug 24, 2020 • edited Loading

billsacks left a comment

Choose a reason for hiding this comment

slevis-lmwg commented Sep 25, 2020

slevis-lmwg commented Sep 25, 2020

slevis-lmwg commented Sep 25, 2020

billsacks commented Sep 25, 2020

slevis-lmwg commented Aug 7, 2020 •

edited

Loading

slevis-lmwg commented Aug 10, 2020 •

edited

Loading

billsacks commented Aug 21, 2020 •

edited by slevis-lmwg

Loading

billsacks Aug 21, 2020 •

edited by slevis-lmwg

Loading

billsacks Aug 24, 2020 •

edited

Loading

billsacks Aug 21, 2020 •

edited by slevis-lmwg

Loading

slevis-lmwg Aug 24, 2020 •

edited

Loading

slevis-lmwg commented Aug 24, 2020 •

edited

Loading