Closed guoqiang-tang closed 1 year ago
Thanks for the PR. Could you please address the following?
@andywood could you please have a look at the changes to the numerical settings in the parameter file? If you can recall why these changes were needed it'd be very helpful if you could outline that here so we can refer to the info later, in case that becomes necessary.
The localParam changes look fine to me ... I'm not sure where the various versions of these came from, tbh. My current localParam settings are not the same as either the old or new versions here, though they match in some places. At some point I recall the range on k_macropore to reach higher values, I think. I support the min canopy height max bound going higher, maybe even more than 15m -- e.g., think Douglas fir ... https://www.nps.gov/articles/images/MORA-DouglasFirForest_Longmire-20171013.jpg?maxwidth=1200&autorotate=false
There may end up being different versions of localParams (& other params/attributes) associated with different test cases, but it would be good to document changes to the main defaults included with the code. This would particularly highlight where the change was a 'fix' for some poor behavior.
I'm not sure code & param file changes should always be separated, especially in the case where code may be added that requires a new parameter, in which case it would be odd to put that into a different PR. But I agree on the documentation aspect, and if the localParam update is totally unconnected from the rest of the PR, no reason to combine them.
I agree with the discussion about localParam. At least for this PR, the code change and localParam change should be separated. I will limit this PR to scalarSnowDrainage and create a localParam PR later.
Thanks Guoqiang, included in a separate PR
Make sure all the relevant boxes are checked (and only check the box if you actually completed the step):
./summa/docs/whats-new.md
or./summa/docs/minor-changes.md
depending on what changed)scalarSnowDrainage=drainageMeltPond/iden_water
is added whennSnow==0
according to Martyn's suggestion.