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.

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

Noah-MP-4.0.1 snow DA updates #1352

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

jupflug
Copy link

@jupflug jupflug commented Jun 9, 2023

Fixes issue #1349

I was unable to change the head repository on PR #1350. I resubmitted here so that the snow DA updates are instead compared versus the lisf-public-7.4 branch. The previous pull request was closed, and will be deleted once this PR is complete.

The first two bugs identified by #1349 were fixed within noahmp401_snow_update.F90:

For cases when the sign (positive/negative) of dsnowh and dsneqv were opposite, dsnowh was recalculated using dsneqv and NoahMP snow density
isnow was updated to the correct number of snow layers for cases when dsneqv and dsnowh were negative. snice was also adjusted for this case, assuming that snliq was zero for all layers

The snow partitioning addition identified by #1349 will be added as a feature in a future PR

@sujayvkumar @karsenau @dmocko

@jupflug jupflug mentioned this pull request Jun 9, 2023
@cmclaug2
Copy link
Contributor

The code compiles with intel and gnu compilers with strict checks.

@sujayvkumar, @karsenau or @dmocko I have reviewed the changes and the conversation on the #1350 thread. To the best of my knowledge, these code changes are acceptable. I would need to learn more about this specific snow model to further validate this PR. I would like to request a second set of eyes on these changes before I accept.

I agree with @dmocko that this should merge into lis-public-7.4 because it fixes a bug on the existing public branch. Going forward, the third bug fix in #1349 should also be worked on in the lisf-public-7.4 support branch.

@jupflug When this is accepted, I will close #1349. For the bug fix regarding the precipitation partitioning schemes, go ahead and open a new issue.

@karsenau
Copy link
Contributor

Hi @cmclaug2 and @dmocko:

Thank you so much for your thorough reviews involved with the #1350 thread and code updates.

I agree that the updates in noahmp401_snow_update.F90 can go to lis-public-7.4, as they should help address some consistency issues in our NoahMP401 snow state updates.

For the 3rd issue, that could go to to lis-public-7.4 or master, as it serves as a fifth option in NoahMP401's configuration (in the lis.config file) to support a snowfall / rainfall discrimination based on some code from SnowModel.

@cmclaug2 cmclaug2 added the help wanted Extra attention is needed label Jun 22, 2023
@cmclaug2
Copy link
Contributor

@sujayvkumar @karsenau @dmocko,

I have taken another look at @jupflug updates.

  • I confirm, "For cases when the sign (positive/negative) of dsnowh and dsneqv were opposite, dsnowh was recalculated using dsneqv".
  • I confirm, snow depth change is set to zero when there is no SWE change.
  • I confirm, when "dsneqv and dsnowh were negative",
    - isnow was updated
    - snice was updated

I will leave this open for further evaluation of the specifc science updates. Otherwise, I am ready to merge!

@jupflug
Copy link
Author

jupflug commented Jul 6, 2023

Thanks for checking and confirming these changes @cmclaug2. I am okay to to merge once we have everyone's OK.

@dmocko
Copy link
Contributor

dmocko commented Jul 19, 2023

Hi all,

I am satisfied that my concerns on the previous (now cancelled) PR have been addressed.

As the @NASA-LIS/lsm rep, I am satisfied with this current PR.

However, I believe that @yyoon4 and/or @sujayvkumar should give their approval to these Noah-MP snow DA updates before this PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants