Bugfixes to SLC checks, post-LIAU#339
Bugfixes to SLC checks, post-LIAU#339ClaraDraper-NOAA wants to merge 4 commits intoufs-community:ufs/devfrom
Conversation
Deleted code associated with already removed print statements Deleted excess comments.
ClaraDraper-NOAA
requested review from
Qingfu-Liu,
barlage,
cenlinhe,
dustinswales,
grantfirl and
rhaesung
as code owners
|
@barlage Here's the PR for the STC check we were discussing yesterday. |
* Change tfreeze to match that in NoahMP * Only set SLC to SMC if temp newly > tfreez
ClaraDraper-NOAA
changed the title
|
Updated PR to also update the value used to determine freezing soils. Some figures showing impact on model output here |
Access to the figures is not open |
Thanks Mike - I just made them visible / commentable to anyone from NOAA. |
|
@ClaraDraper-NOAA Is this PR ready for review and the UFS merge queue? I'm not seeing any supermodule PRs or regression testing that has been done, although I see that you tested in the global workflow on Ursa. Do you plan to open supermodule PRs and run the regression tests yourself? |
| integer :: nother, nsnowupd | ||
| integer :: nstcupd, nslcupd, nfrozen, nfrozen_upd | ||
| logical :: print_update_stats = .False. | ||
| real(kind=kind_phys), parameter :: tfreez_noahmp=273.16 ! tfreez used in NoahMP to determine frozen ground |
There was a problem hiding this comment.
Is there a reason why noahmp needs to redefine this constant? The host provides this value as a physical constant and it should be used to maintain consistency with the rest of physics.
There was a problem hiding this comment.
It looks like con_t0c (temperature_at_zero_celsius) is set to 273.15 and con_ttp (triple_point_temperature_of_water) is set to 273.16.
There was a problem hiding this comment.
These are really questions for the land modeling folks. @barlage do you know why Noah-MP uses the slightly higher tfreeze? I'm not keen on using con_ttp here unless Noah-MP is using 273.16 specifically because it is the triple point (as in, this would be the right answer for the wrong reason, which could create problems later on). Also, do you know whyN Noah-MP is not taking the tfreeze constant from the ufs model? If it's just a matter of it hasn't been done yet, we could look into what would be involved to update that now.
There was a problem hiding this comment.
It's a little strange that this PR is going from using con_t0c that is already consistent with the rest of physics and redefining it locally. I guess it's OK to define tfreez_noahmp, but it should be set from the host, either from con_t0c or con_ttp, IMO.
There was a problem hiding this comment.
The problem is that the freezing temp that is used by the NoahMP model (in the noahmp_sflx routine) is set to 273.16 in the model code locally here. con_t0c is set in the driver, but is not passed through to the NoahMP model call. The change in this PR brings the same value into the DA since the model and DA values must be the same.
An alternative to my fix is to pass the con_t0c value through to the NoahMP routine (and remove both local tfreez definitions), but this is a science change to the model. @barlage what do you think?
There was a problem hiding this comment.
@ClaraDraper-NOAA From a CCPP perspective, your second method of passing con_t0c through should be implemented. Shouldn't physical constants be consistent across all model components, including DA (shouldn't DA receive physical constants from the host too?). I don't really view this as a science change, IMO.
There was a problem hiding this comment.
This would be a science change, because the freezing constant in the model would change from 273.16 to 273.15 K.
The issue I'm solving here is that the freezing points used by the DA and the land model are different. We need to fix this as the DA is currently melting soil moisture points that fall between these two values (repeatedly, at every time step!), which is messing with the energy balance at those points. If you want the model to inherit the freezing point from con_t0c then I suggest you address this as a separate issue. Once that's done, I can make then update the DA code to match.
|
Most of the code changes look fine. I'll approve once the question about the freezing point constant is addressed and regression testing has been done. |
I can prepare the submodule super PRs. In general, what is the expectation for this? These changes are not urgent (not being used in v17). Is the usual process that I prepare all the ufs supermodules PRs, or is it sufficient to do a test (like the global_workflow one I did), and then allow these changes to be merged into the supermodules next time the hash is updated? |
|
@ClaraDraper-NOAA To be clear, this PR isn't directly related to #343 and others that you have forthcoming? If so, either you as the developer or code managers (me or someone else) need to put in supermodule PRs, even if they are nothing more than commit hash updates. In order for anything to be merged into this ccpp-physics branch, it will need to have all UFS RTs performed as well, not just testing in the global workflow. Let me and/or @rhaesung know if you'd like help adding the supermodule PRs or running the UFS RTs. |
You are correct - these updates are no related to #343 in any way. I'll get on to do the supermodule changes and testing (note, I'm travelling next week, so may not get it done until I'm back). |
3 participants
Description of Changes:
Increased the lower bound allowed for soil moisture from 1 mm/layer to 0.1 mm/layer.
Also changed logic to allow soil moisture to be below the lower bound, but to prevent the DA increments themselves from reducing soil moisture below the lower bound.
The latter required moving the check into the code block where the increment is applied (and out of the do_stcsmc_adjustment conditional. This should really always be set to true, so I think this is OK.
Additional tidy up:
-removed old code monitoring the different types of updates, since the print statements for the results have been removed
-removed some excessive comments
Tests Conducted:
Compile and ran in global_workflow/ufs_model on URSA. results are as expected. The blocks of reduced sfc temps in very arid regions are now gone.
Dependencies:
N/A
Documentation:
N/A
Issue (optional):
Resolves issue #