search

CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:
http://www.ral.ucar.edu/projects/summa
GNU General Public License v3.0
80 stars 105 forks source link

Feature/snow layer creation threshold #520

Closed wknoben closed 1 year ago

wknoben commented 1 year ago

Make sure all the relevant boxes are checked (and only check the box if you actually completed the step):

Test cases

Synthetic test cases complete bit-for-bit identical.

WRR test cases do not, because changes to the snow layering scheme translate into changes in simulated snow (and related) variables. See example below. Using the new layering scheme (bottom plot, dashed lines in both plots) can lead to lower, identical and higher snow depths than the original scheme gives us. Differences are in the order of centimeters over a multi-year simulation (second figure).

WIP: add an explanation about why the snow sims are slightly different.

summa_snowLayer_PR

summa_snowLayer_PR_depthDiff

andywood commented 1 year ago

Just curious, is the layer creation threshold hardwired? I haven't looked at the SUMMA code in a while. Since this changes the performance of the model, the version update would have the potential to require recalibration for existing implementations. If the threshold becomes a user parameter, some users could keep it the same or jump to a new default.

On Tue, Mar 7, 2023 at 11:38 AM Wouter Knoben @.***> wrote:

Make sure all the relevant boxes are checked (and only check the box if you actually completed the step):

  • Closes #xxx (identify the issue associated with this PR)
  • Code passes standard test cases (results are either bit-for-bit identical, or differences are explained in the PR comment)
  • New tests added (describe which tests were performed to test the changes)
  • Science test figures (add figures to PR comment and describe the tests)
  • Checked that the new code conforms to the SUMMA coding conventions https://github.com/NCAR/summa/blob/master/docs/howto/summa_coding_conventions.md
  • Describe the change in the release notes (use either ./summa/docs/whats-new.md or ./summa/docs/minor-changes.md depending on what changed)

Test cases

Synthetic test cases complete bit-for-bit identical.

WRR test cases do not, because changes to the snow layering scheme translate into changes in simulated snow (and related) variables. See example below. Using the new layering scheme (bottom plot, dashed lines in both plots) can lead to lower, identical and higher snow depths than the original scheme gives us. Differences are in the order of centimeters over a multi-year simulation (second figure).

WIP: add an explanation about why the snow sims are slightly different.

[image: summa_snowLayer_PR] https://user-images.githubusercontent.com/45757529/223516145-7d998308-4bf1-468a-a6a4-c74f7a5e27c1.png

[image: summa_snowLayer_PR_depthDiff] https://user-images.githubusercontent.com/45757529/223518198-c56f26eb-7b39-4c7c-88e8-c9bcdffedf7b.png

You can view, comment on, or merge this pull request online at:

https://github.com/CH-Earth/summa/pull/520 Commit Summary

File Changes

(2 files https://github.com/CH-Earth/summa/pull/520/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/520, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARM5SWRNSB672ZV6XCDW256BTANCNFSM6AAAAAAVS2WLAQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

wknoben commented 1 year ago

It's a combination of hard-wired logic and user-defined parameters in localParamInfo.txt.

Logic lines - layerDivide():

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerDivide.f90#L172-L188

[...]

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerDivide.f90#L260-L284

Logic lines - layerMerge():

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerMerge.f90#L136-L137

[...]

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerMerge.f90#L158-L173

User-specified parameters:

zmin                      |       0.0100 |       0.0050 |       0.1000
zmax                      |       0.0500 |       0.0100 |       0.5000
! ---
zminLayer1                |       0.0075 |       0.0075 |       0.0075
zminLayer2                |       0.0100 |       0.0100 |       0.0100
zminLayer3                |       0.0500 |       0.0500 |       0.0500
zminLayer4                |       0.1000 |       0.1000 |       0.1000
zminLayer5                |       0.2500 |       0.2500 |       0.2500
! ---
zmaxLayer1_lower          |       0.0500 |       0.0500 |       0.0500
zmaxLayer2_lower          |       0.2000 |       0.2000 |       0.2000
zmaxLayer3_lower          |       0.5000 |       0.5000 |       0.5000
zmaxLayer4_lower          |       1.0000 |       1.0000 |       1.0000
! ---
zmaxLayer1_upper          |       0.0300 |       0.0300 |       0.0300
zmaxLayer2_upper          |       0.1500 |       0.1500 |       0.1500
zmaxLayer3_upper          |       0.3000 |       0.3000 |       0.3000
zmaxLayer4_upper          |       0.7500 |       0.7500 |       0.7500

I'm not a 100% sure but I expect it may be difficult (if not impossible) to mimic the current behavior using the new code and custom parameter values. What are your thoughts on this? Is requiring recalibration a reason to not make this change?

andywood commented 1 year ago

Ah, I misunderstood - if it's a logic change rather than a threshold setting change then there could well be a good reason to make it. I didn't see the reason for the change earlier (a bug in the old layering scheme? more stability?).

On Wed, Mar 8, 2023 at 10:09 AM Wouter Knoben @.***> wrote:

It's a combination of hard-wired logic and user-defined parameters in localParamInfo.txt.

Logic lines - layerDivide():

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerDivide.f90#L172-L188

[...]

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerDivide.f90#L260-L284

Logic lines - layerMerge():

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerMerge.f90#L136-L137

[...]

https://github.com/CH-Earth/summa/blob/a34af362b9a9b6c09e72d1e822d58e95ccfa2713/build/source/engine/layerMerge.f90#L158-L173

User-specified parameters:

zmin | 0.0100 | 0.0050 | 0.1000 zmax | 0.0500 | 0.0100 | 0.5000 ! --- zminLayer1 | 0.0075 | 0.0075 | 0.0075 zminLayer2 | 0.0100 | 0.0100 | 0.0100 zminLayer3 | 0.0500 | 0.0500 | 0.0500 zminLayer4 | 0.1000 | 0.1000 | 0.1000 zminLayer5 | 0.2500 | 0.2500 | 0.2500 ! --- zmaxLayer1_lower | 0.0500 | 0.0500 | 0.0500 zmaxLayer2_lower | 0.2000 | 0.2000 | 0.2000 zmaxLayer3_lower | 0.5000 | 0.5000 | 0.5000 zmaxLayer4_lower | 1.0000 | 1.0000 | 1.0000 ! --- zmaxLayer1_upper | 0.0300 | 0.0300 | 0.0300 zmaxLayer2_upper | 0.1500 | 0.1500 | 0.1500 zmaxLayer3_upper | 0.3000 | 0.3000 | 0.3000 zmaxLayer4_upper | 0.7500 | 0.7500 | 0.7500

I'm not a 100% sure but I expect it may be difficult (if not impossible) to mimic the current behavior using the new code and custom parameter values. What are your thoughts on this? Is requiring recalibration a reason to not make this change?

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/520#issuecomment-1460517883, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARJZOQ23BALX2LQFVQ3W3C4ODANCNFSM6AAAAAAVS2WLAQ . You are receiving this because you commented.Message ID: @.***>

wknoben commented 1 year ago

This change should lead to more intuitive logic, but you're right that the PR doesn't mention this yet. I'm still working on it.

What we currently have is the following:

If we then look at the case where there are snow layers, the logic is:

So essentially with the current logic, the conditions that lead to creating the first layer would lead to subdividing that layer on the next time step if nothing changes. In other words, the same conditions lead to a different number of snow layers depending on if there are 0 or 1 snow layers when the check is made.

This change should lead to more consistent logic in how many snow layers you get for a given snow depth.

andywood commented 1 year ago

Great change, that will certainly be cleaner :)

On Wed, Mar 8, 2023 at 10:51 AM Wouter Knoben @.***> wrote:

This change should lead to more intuitive logic, but you're right that the PR doesn't mention this yet. I'm still working on it.

What we currently have is the following:

  • If there are no snow layers (nSnow == 0),
  • And "snow-without-a-layer" (stored in scalarSnowDepth) is larger than the maximum depth we want the first layer to have ( zmaxLayer1_lower),
  • Make a new layer.

If we then look at the case where there are snow layers, the logic is:

  • If there is exactly one snow layer,
  • Set the threshold for sub-dividing this layer to zmaxLayer1_lower
  • Then, if mLayerDepth for that 1 layer is > zmaxLayer1_lower,
  • Subdivide the layer.

So essentially with the current logic, the conditions that lead to creating the first layer would lead to subdividing that layer on the next time step if nothing changes. In other words, the same conditions lead to a different number of snow layers depending on if there are 0 or 1 snow layers when the check is made.

This change should lead to more consistent logic in how many snow layers you get for a given snow depth.

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/520#issuecomment-1460587949, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARJBILUWUGH7BLY6T2DW3DBLRANCNFSM6AAAAAAVS2WLAQ . You are receiving this because you commented.Message ID: @.***>