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

Snow compaction #148

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Snow compaction #148

wants to merge 1 commit into from

Conversation

RAbolafiaRosenzweig
Copy link
Collaborator

Enables new snow compaction physics option using the enhanced scheme developed by Abolafia-Rosenzweig et al. (2024). This new physics option can be used by defining SNOW_COMPACTION_OPTION=2 in the namelist file.

Abolafia-Rosenzweig, R., He, C., Chen, F., Barlage, M., 2024. Evaluating and Enhancing Snow Compaction Process in the Noah-MP Land Surface Model. J Adv Model Earth Syst 16, e2023MS003869. https://doi.org.10.1029/2023MS003869

@cenlinhe
Copy link
Collaborator

Hi Ronnie, a few minor suggestions:

  1. please remove "!RAR" from the code.
  2. Include your paper as reference in your new compaction module here: https://github.com/NCAR/noahmp/pull/148/files#diff-0b59d5fc1c3d226094efe4310ffa808aad49ab623ecacd8c6014fe5a5a3f1ff6R16-R20
  3. line up the code for formatting. Some code lines are not lined up with the existing code.
  4. You currently have three "snow_compaction" commits. Could you please just use 1 commit to include all of your code updates here? For example, two of your current commits just include formatting changes, which are not worth separate commits.

Thanks!

@cenlinhe cenlinhe requested review from tslin2 and cenlinhe October 16, 2024 16:12
@RAbolafiaRosenzweig RAbolafiaRosenzweig force-pushed the snow_compaction branch 3 times, most recently from 47128ba to aaee8fe Compare October 16, 2024 16:22
@RAbolafiaRosenzweig
Copy link
Collaborator Author

These 4 suggestions have been accepted & incorporated.

@cenlinhe cenlinhe requested a review from dmocko October 16, 2024 17:15
tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/ConfigVarType.F90 Outdated
@@ -121,6 +121,10 @@ module ConfigVarType
! 1 -> include phase change of ice (default)
! 2 -> ice treatment more like original Noah

integer :: OptSnowCompact ! options for ground snow compaction
! 1 -> original scheme from Anderson (1976)
! 2 -> enhanced scheme from Abolafia-Rosenzweig et al. (2023)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps change (2023) to (2024)

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/Makefile Outdated
@@ -42,6 +42,7 @@ OBJS = ConstantDefineMod.o \
SnowLayerDivideMod.o \
SnowLayerWaterComboMod.o \
SnowpackCompactionMod.o \
SnowpackCompactionAR2023Mod.o \
Copy link
Collaborator

@tslin2 tslin2 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also include the Dependencies as in the below of this file, e.g. in line 208
SnowpackCompactionMod.o: ../utility/Machine.o NoahmpVarType.o ConstantDefineMod.o

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be 2024 not 2023

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated
@@ -0,0 +1,147 @@
module SnowpackCompactionARMod
Copy link
Collaborator

@tslin2 tslin2 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to have a consistent file name and module name, which are consistent with other files naming in Noah-MPv5.0.
e.g., change SnowpackCompactionAR2023Mod.F90 to SnowpackCompactionARMod.F90

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated

! start snow compaction
SnowBurden = 0.0
!Added by Ronnie (SnowCompactBurdenFac updated from Abolafia-Rosenzweig et al., 2023)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 2024

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/SnowWaterMainMod.F90 Outdated
@@ -64,7 +66,9 @@ subroutine SnowWaterMain(noahmp)
! do following snow layer compaction, combination, and division only for multi-layer snowpack

! snowpack compaction
if ( NumSnowLayerNeg < 0 ) call SnowpackCompaction(noahmp)

if ( NumSnowLayerNeg < 0 .AND. OptSnowCompact == 1) call SnowpackCompaction(noahmp)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be capitalized, other modules use .and.

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated
!Added by Ronnie (SnowCompactBurdenFac updated from Abolafia-Rosenzweig et al., 2023)
SnowCompactBurdenFac = -0.00069503 * TemperatureAirRefHeight + 0.20606699
!pressure-based lower constraints:
IF (PressureAirRefHeight>=85000) THEN !high pressure bin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF, THEN, ENDIF, MAX, MIN, .AND. should not be uppercase; other parts of codes are lowercase

tslin2
tslin2 reviewed Nov 14, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated
SnowCompactBurdenFac = -0.00069503 * TemperatureAirRefHeight + 0.20606699
!pressure-based lower constraints:
IF (PressureAirRefHeight>=85000) THEN !high pressure bin
SnowCompactBurdenFac = MAX(SnowCompactBurdenFac,0.017) !this lower bound should never be triggered
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX is min in the paper

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misunderstand; the paper is the binning of minimum

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure; if I see figure S4 in the paper, 0.017 is the upper bin

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 0.017 be in the Noah-MP parameter table?

@tslin2
Copy link
Collaborator

tslin2 commented Nov 14, 2024 •
edited
Loading

Should this enhancement of SnowCompactBurdenFac to be considered an option in the Noah-MP parameter table or a new physics option in the namelist? One physics, but two options for the SnowCompactBurdenFac parameter (original and enhanced) in the Noah-MP parameter table? Thanks.

tslin2
tslin2 reviewed Nov 15, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated
! start snow compaction
SnowBurden = 0.0
!Added by Ronnie (SnowCompactBurdenFac updated from Abolafia-Rosenzweig et al., 2023)
SnowCompactBurdenFac = -0.00069503 * TemperatureAirRefHeight + 0.20606699
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these two numeric values be in the Noah-MP parameter table?

tslin2
tslin2 reviewed Nov 15, 2024
View reviewed changes
src/SnowpackCompactionAR2023Mod.F90 Outdated
SnowCompactBurdenFac = MAX(SnowCompactBurdenFac,0.019) !this lower bound should never be triggered
ENDIF
!upper constraint on SnowCompactBurdenFac
SnowCompactBurdenFac = MIN(SnowCompactBurdenFac,0.0315)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 0.0315 be in the Noah-MP parameter table?

@RAbolafiaRosenzweig
snow_compaction
d411268 
snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction

snow_compaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tslin2 tslin2 tslin2 left review comments

@cenlinhe cenlinhe Awaiting requested review from cenlinhe

@dmocko dmocko Awaiting requested review from dmocko

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RAbolafiaRosenzweig @cenlinhe @tslin2