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.

Sign up for GitHub

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

Jump to bottom

Rename frac_sno and frac_sno_eff #822

Open
billsacks opened this issue Oct 17, 2019 · 1 comment
Open

Rename frac_sno and frac_sno_eff #822

billsacks opened this issue Oct 17, 2019 · 1 comment
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability) good first issue simple; good for first-time contributors

Comments

@billsacks
Copy link
Member

It's hard to keep track of the difference between frac_sno and frac_sno_eff. @swensosc suggests the following rename to show their primary / intended purposes in the code; I like his suggestion:

  • Rename frac_sno to frac_sno_albedo

  • Rename frac_sno_eff to frac_sno_fluxes

Note that frac_sno is used in a few places that aren't related to albedo / radiation. Looking back at the clm4.0 code (from before frac_sno_eff was introduced), I see uses of frac_sno in:

  • DUSTMod: lnd_frc_mbl
  • Biogeophysics1: qred and soilbeta

I haven't checked the latest code to confirm that those are still the only uses of frac_sno, but from talking to @swensosc , I think the intent was that most of the new code that depends on snow cover fraction (for the sake of calculating sub-column fluxes) would use frac_sno_eff.

Also, note that there are (or at least were in the past) some associates like frac_sno => frac_sno_eff, so we need to be careful when doing this rename. Probably a good first step would be to find those associates and fix them so that the name on the left matches the name on the right.
@billsacks billsacks added the code health improving internal code structure to make easier to maintain (sustainability) label Oct 17, 2019
billsacks added a commit to billsacks/ctsm that referenced this issue Oct 25, 2019
@billsacks
Consistently use frac_sno_eff rather than frac_sno in qg calculations
957a1d4 
The change here only changes answers for (1) urban pervious road, and
(2) more broadly with use_subgrid_fluxes = .false.

Previously, code in CalculateSurfaceHumidity was inconsistent regarding
the use of frac_sno vs. frac_sno_eff. I wanted to make this consistent
to facilitate upcoming cleanup of CalculateSurfaceHumidity. I went back
and forth as to which one we should use here. (See also
ESCOMP#822).

Argument for frac_sno_eff: This is consistent with the use of
frac_sno_eff elsewhere in the calculation of surface fluxes.

Argument for frac_sno: Even in CLM4.0 (prior to the introduction of
frac_sno_eff and frac_h2osfc), frac_sno was used to calculate surface
relative humidity. That makes me think that the parameterizations were
improved by using frac_sno here, even though frac_sno at that point was
mainly just used for albedo / radiation calculations. So sticking with
frac_sno would be more consistent with what has been done for a long
time, and would presumably be a bit better scientifically.

However, when I tried changing this to use frac_sno rather than
frac_sno_eff:

    qg(c) = frac_sno_eff(c)*qg_snow(c) + (1._r8 - frac_sno_eff(c) - frac_h2osfc(c))*qg_soil(c) &
          + frac_h2osfc(c) * qg_h2osfc(c)

I got a crash with use_subgrid_fluxes false (test
SMS_D_Ld1_P4x1.f10_f10_musgs.I2000Clm45BgcCropQianRsGs.bishorn_gnu.clm-no_subgrid_fluxes),
due to a check I put in relatively recently to ensure that the top-layer
soil liquid never goes significantly negative:

WJS 218: frac_sno, frac_sno_eff, frac_h2osfc, snl =    6.6447358408778726E-005   1.0000000000000000        0.0000000000000000               -1
 qg_snow, qg_soil, qg_h2osfc, qg =    3.8414454029812475E-003   3.6634834595404343E-003   3.6635176705822736E-003   3.6634952846414733E-003
 ERROR: In UpdateState_TopLayerFluxes, h2osoi_liq has gone significantly negative
 Bulk/tracer name = bulk
 c, lev_top(c) =          218           0
 h2osoi_liq_top_orig  =    6.6448689668604415E-004
 h2osoi_liq           =   -2.6754040213948407E-004
 frac_sno_eff         =    1.0000000000000000
 qflx_liq_grnd*dtime  =    0.0000000000000000
 qflx_dew_grnd*dtime  =    0.0000000000000000
 qflx_evap_grnd*dtime =    9.3202729882552822E-004
 ENDRUN:
 ERROR: In UpdateState_TopLayerFluxes, h2osoi_liq has gone significantly negative

What seems to be going on is: With a little bit of snow present, when I
use frac_sno rather than frac_sno_eff, we end up with a qg value very
close to that of soil. But I think the later evaporation happens from
the snow, at which point I guess the dryer air above leads to there
being too large evaporation from the snow.

So in the end, I decided to consistently use frac_sno_eff, even though
this might be a small step backwards in terms of the accuracy of surface
humidity for urban pervious road and with use_subgrid_fluxes = .false.
billsacks added a commit that referenced this issue Oct 27, 2019
@billsacks
Merge branch 'qg_frac_sno_eff'
7e13a6e 
Consistently use frac_sno_eff rather than frac_sno in qg calculations

The change here only changes answers for (1) urban pervious road, and
(2) more broadly with use_subgrid_fluxes = .false.

Previously, code in CalculateSurfaceHumidity was inconsistent regarding
the use of frac_sno vs. frac_sno_eff. I wanted to make this consistent
to facilitate upcoming cleanup of CalculateSurfaceHumidity. I went back
and forth as to which one we should use here. (See also
#822).

Argument for frac_sno_eff: This is consistent with the use of
frac_sno_eff elsewhere in the calculation of surface fluxes.

Argument for frac_sno: Even in CLM4.0 (prior to the introduction of
frac_sno_eff and frac_h2osfc), frac_sno was used to calculate surface
relative humidity. That makes me think that the parameterizations were
improved by using frac_sno here, even though frac_sno at that point was
mainly just used for albedo / radiation calculations. So sticking with
frac_sno would be more consistent with what has been done for a long
time, and would presumably be a bit better scientifically.

However, when I tried changing this to use frac_sno rather than
frac_sno_eff:

    qg(c) = frac_sno_eff(c)*qg_snow(c) + (1._r8 - frac_sno_eff(c) - frac_h2osfc(c))*qg_soil(c) &
          + frac_h2osfc(c) * qg_h2osfc(c)

I got a crash with use_subgrid_fluxes false (test
SMS_D_Ld1_P4x1.f10_f10_musgs.I2000Clm45BgcCropQianRsGs.bishorn_gnu.clm-no_subgrid_fluxes),
due to a check I put in relatively recently to ensure that the top-layer
soil liquid never goes significantly negative:

WJS 218: frac_sno, frac_sno_eff, frac_h2osfc, snl =    6.6447358408778726E-005   1.0000000000000000        0.0000000000000000               -1
 qg_snow, qg_soil, qg_h2osfc, qg =    3.8414454029812475E-003   3.6634834595404343E-003   3.6635176705822736E-003   3.6634952846414733E-003
 ERROR: In UpdateState_TopLayerFluxes, h2osoi_liq has gone significantly negative
 Bulk/tracer name = bulk
 c, lev_top(c) =          218           0
 h2osoi_liq_top_orig  =    6.6448689668604415E-004
 h2osoi_liq           =   -2.6754040213948407E-004
 frac_sno_eff         =    1.0000000000000000
 qflx_liq_grnd*dtime  =    0.0000000000000000
 qflx_dew_grnd*dtime  =    0.0000000000000000
 qflx_evap_grnd*dtime =    9.3202729882552822E-004
 ENDRUN:
 ERROR: In UpdateState_TopLayerFluxes, h2osoi_liq has gone significantly negative

What seems to be going on is: With a little bit of snow present, when I
use frac_sno rather than frac_sno_eff, we end up with a qg value very
close to that of soil. But I think the later evaporation happens from
the snow, at which point I guess the dryer air above leads to there
being too large evaporation from the snow.

So in the end, I decided to consistently use frac_sno_eff, even though
this might be a small step backwards in terms of the accuracy of surface
humidity for urban pervious road and with use_subgrid_fluxes = .false.
@negin513
Copy link
Contributor

As @billsacks mentioned there are some associates like frac_sno => frac_sno_eff in some modules. These need to be handled and fixed first before renaming frac_sno to frac_sno_albedo and frac_sno_eff to frac_sno_fluxes.

So far, I've found the following modules that have associates like frac_sno => frac_sno_eff.

biogeophys/SnowHydrologyMod.F90
     subroutine SnowCompaction
     subroutine DivideSnowLayers

 biogeophys/SoilTemperatureMod.F90
     subroutine SoilThermProp
     subroutine PhaseChangeH2osfc

biogeophys/SoilHydrologyMod.F90
     subroutine SetQflxInputs

biogeophys/SnowSnicarMod.F90
     subroutine SNICAR_RT
     subroutine SnowAge_grain

biogeophys/CanopyFluxesMod.F90
     subroutine CanopyFluxes

@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 9, 2023
@billsacks billsacks removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Mar 2, 2023
@samsrabin samsrabin added simple bfb bit-for-bit and removed simple bfb labels Aug 8, 2024
@samsrabin samsrabin added good first issue simple; good for first-time contributors and removed simple labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability) good first issue simple; good for first-time contributors
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@billsacks @samsrabin @ekluzek @negin513