Closed olyson closed 9 months ago
From discussion at ctsm-software meeting: general agreement about making this change, but we'd like input from @swensosc . But should we rename variables in the code to make this more clear? For example, what is now "sabg_chk" could be "sabg", and then "sabg" could become "sabg_internal" or something like that?
I think the issue here is that the frac_sno can be modified within the time step and the solar applied in SoilTemperature depends on the current value of frac_sno, which may lead to an inconsistency with the previously calculated value of sabg. The name should be changed, but I think the problem is that the subgrid fluxes are used for some land unit types but not others. It may be that sabg_chk could be removed, and sabg could simply be updated at that point in the code instead. I am not sure, but that might give the same results. If it didn't give the same results, I would have to spend some time to better understand what is happening. The joys of reviewing code ten years too late...
I removed sabg_chk (I used sabg directly in SoilTemperatureMod) and the model ran fine for 1 month in a global simulation. The only history field that had a difference was SABG (which now has the correct sabg). Getting FSA to output the correct absorbed solar was a bit more involved since fsa is calculated in SurfaceRadiationMod, before SoilTemperature. So I came up with this bit of code in SoilTemperature:
! Recalculate sabg, in case frac_sno is set to zero later, needed for balance check
sabg_fsno = frac_sno_eff(c) * sabg_snow(p) + (1._r8 - frac_sno_eff(c) ) * sabg_soil(p)
! Adjust the diagnostics fsa, fsr, and fsa_r, calculated in SurfaceRadiationMod for any change in sabg
fsa(p) = fsa(p) + (sabg(p) - sabg_fsno)
fsr(p) = fsr(p) - (sabg(p) - sabg_fsno)
if (lun%itype(l)==istsoil .or. lun%itype(l)==istcrop) then
fsa_r(p) = fsa_r(p) + (sabg(p) - sabg_fsno)
end if
! Update sabg
sabg(p) = sabg_fsno
Note that I also had to adjust fsr (so that the solar radiation balance check passed) and fsa_r (the rural version of fsa; there is no rural version of fsr).
The history fields that changed are:
ERRSOL, FSA, FSA_R, FSR, SABG
which are all diagnostics fields, with one exception. FSA is passed to the atmosphere so presumably that would change answers in a coupled simulation. But this new FSA is the correct solar absorbed. Now I'm also wondering if the original could have been a source of energy imbalance from the land in the coupled system. Unfortunately, we also pass the vis/nir and direct/diffuse albedos to the atmosphere and this would not be reflected in my changes. So solar absorbed and albedo are now inconsistent.
One could also go down the rabbit hole further for variables like, for example, FSRVD, FSRND, FSRVI, FSRNI. But one would have to make some assumption about how the adjustment to sabg is split between vis/nir and direct/diffuse. At that point we would just be making stuff up.
This fixes the original problem of FSA and SABG (and FSR) not indicating the correct solar absorbed, and those history fields are now consistent with the energy balance check in the model. However, the solar absorbed and albedos that are passed to the atmosphere are now inconsistent. I'm not sure which situation is worse!
Thanks a lot for your work on this, @olyson ! I have a couple of questions:
This fixes the original problem of FSA and SABG (and FSR) not indicating the correct solar absorbed, and those history fields are now consistent with the energy balance check in the model. However, the solar absorbed and albedos that are passed to the atmosphere are now inconsistent. I'm not sure which situation is worse!
Is it worth trying to find out how CAM uses these fields? I'm wondering why it wants FSA (swnet) from CLM when it already has albedos. For what it's worth, the WRF coupling does not use FSA / swnet (unless it has been added recently).
One could also go down the rabbit hole further for variables like, for example, FSRVD, FSRND, FSRVI, FSRNI. But one would have to make some assumption about how the adjustment to sabg is split between vis/nir and direct/diffuse. At that point we would just be making stuff up.
Does that mean that these diagnostics are now inconsistent with the ones you have fixed. i.e., is the sum (FSRVD + FSRND + FSRVI + FSRNI)
no longer equal to FSR
? If so, is that a problem?
I have a vague recollection that CAM uses FSA for a diagnostic calculation, but I'll have to check into that.
Yes, the FSRVD + FSRND + FSRVI + FSRNI is now inconsistent with FSR. I think that is a problem for analysis from history output, but not sure how to resolve that, unless we make some assumption about how the adjustment to SABG is split among those components.
The most robust solution to all of this may be to somehow find a way to avoid the adjustment to sabg...
It doesn't look like CAM uses swnet. It seems like it may be used to compute a coupler energy budget diagnostic for all the components.
Closing this as a won't fix. It's not clear how to resolve this from the discussion above.
Brief summary of bug
The variable sabg is used for SABG history output and for FSA (sabg+sabv). Seems as if it should be sabg_chk so that energy from history output balances and SABG and FSA are accurate.
General bug information
CTSM version you are using: release-clm5.0.34-2-ga2989b04 (or any CLM tag when sabg_chk was first introduced)
Does this bug cause significantly incorrect results in the model's science? No, this just affects analysis of results using history output.
Configurations affected: All
Details of bug
Energy for non-urban patches balances in CLM as:
errseb(p) = sabv(p) + sabg_chk(p) + forc_lwrad(c) - eflx_lwrad_out(p) &
Note that sabg_chk is used instead of sabg. This presumably should correspond to the following in history output:
ERRSEB = SABV + SABG + FLDS - FIRE - FSH - EFLX_LH_TOT - FGR
However, this doesn't balance in certain locations because sabg is used instead of sabg_chk for SABG. Similarly, the following doesn't balance either:
ERRSEB = FSA + FLDS - FIRE - FSH - EFLX_LH_TOT - FGR
because FSA = sabg + sabv
If sabg_chk is the true energy absorbed by the ground then it seems like we should be reporting that.