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

NoahMP bug fix #1767

Merged
merged 10 commits into from
Oct 4, 2022
Merged

NoahMP bug fix #1767

merged 10 commits into from
Oct 4, 2022

Conversation

cenlinhe
Copy link
Contributor

@cenlinhe cenlinhe commented Sep 12, 2022

A few bug fixes for NoahMP

TYPE: bug fix

KEYWORDS: two stream inout variable, soil ice in snow combine, water variable unit, generic nsoil treatment

SOURCE: Cenlin He (NCAR) with report from CWB/Taiwan

DESCRIPTION OF CHANGES:
A few bug fixes for NoahMP:

  1. Change a few "out" only variables in TWOSTREAM noahmp subroutine to "inout";
  2. Change the unit for 3 accumulated noahmp water flux variables in comments and registry;
  3. Change the soil ice treatment in subroutine COMBINE to conserve water;
  4. Change some hard-coded "4" soil layer number to "nsoil" for more generic treatment.

LIST OF MODIFIED FILES: list of changed files
M Registry/registry.noahmp
M phys/noahmp

TESTS CONDUCTED:

  1. The bug fixes have been tested for NoahMP run over the U.S. for an entire year.
  2. Reg tests have passed.

RELEASE NOTE: This PR fixes a few variable declaration in subroutine TWOSTREAM from OUT to INOUT, changes soil ice treatment in subroutine COMBINE, and corrects units for three accumulated flux variables in comments and registry. It also removed hard-coded 4 soil layers, and replaces it with parameter NSOIL.

@cenlinhe cenlinhe requested review from a team as code owners September 12, 2022 02:46
@weiwangncar
Copy link
Collaborator

The Jenkins tests have passed:

Test Type | Expected | Received | Failed
= = = = = = = = = = = = = = = = = = = = = = = = = = = =
Number of Tests : 23 24
Number of Builds : 60 58
Number of Simulations : 158 156 0
Number of Comparisons : 95 92 0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@weiwangncar
Copy link
Collaborator

@cenlinhe Would you like to make this change available sooner? That would mean 4.4.2.

@cenlinhe
Copy link
Contributor Author

@weiwangncar Yes, it works for me. Thanks!

@weiwangncar weiwangncar changed the base branch from develop to release-v4.4.2 September 12, 2022 16:16
@cenlinhe
Copy link
Contributor Author

cenlinhe commented Oct 2, 2022

When will this PR be merged to develop or release-v4.4.2 branch? Recently, two other NoahMP bugs have been identified and I would like to create another PR for those bug fixes based on the updated develop or release-v4.4.2 branch with the current PR bug fix included. Thanks!

@weiwangncar
Copy link
Collaborator

@dudhia Can you check this PR and approve if it is OK so that Cenlin can move to his next change?

@dudhia
Copy link
Collaborator

dudhia commented Oct 4, 2022

I also saw some dimension changes for nsoil. Were those described?

@cenlinhe
Copy link
Contributor Author

cenlinhe commented Oct 4, 2022

Those are just changing the hard-coded "4" soil layers to "nsoil". By default, NoahMP uses 4 soil layers, so this change is no-answer change. However, this gives more flexibility for users to change nsoil in the future. Otherwise, users need to manually change the hard-coded "4" soil layer in those code files.

@dudhia
Copy link
Collaborator

dudhia commented Oct 4, 2022

OK, it should be added to the description. Thanks.

@cenlinhe
Copy link
Contributor Author

cenlinhe commented Oct 4, 2022

Just added to the description above.

@weiwangncar
Copy link
Collaborator

@cenlinhe Can you clarify what 'snow combine subroutine' is? This is in your description of change #3.

@cenlinhe
Copy link
Contributor Author

cenlinhe commented Oct 4, 2022

It is this subroutine "COMBINE" in module_sf_noahmplsm.F. I have included this in the above description.

@weiwangncar
Copy link
Collaborator

@cenlinhe I added RELEASE NOTE in your PR. Please see if it is ok.

@cenlinhe
Copy link
Contributor Author

cenlinhe commented Oct 4, 2022

The release note looks good to me. Thanks!

@weiwangncar weiwangncar merged commit 760418b into wrf-model:release-v4.4.2 Oct 4, 2022
vlakshmanan-scala pushed a commit to scala-computing/WRF that referenced this pull request Apr 4, 2024
TYPE: bug fix

KEYWORDS: two stream inout variable, soil ice in snow combine, water variable unit, generic nsoil treatment

SOURCE: Cenlin He (NCAR) with report from CWB/Taiwan

DESCRIPTION OF CHANGES:
A few bug fixes for NoahMP:
1. Change a few "out" only variables in TWOSTREAM noahmp subroutine to "inout";
2. Change the unit for 3 accumulated noahmp water flux variables in comments and registry;
3. Change the soil ice treatment in subroutine COMBINE to conserve water;
4. Change some hard-coded "4" soil layer number to "nsoil" for more generic treatment.

LIST OF MODIFIED FILES: list of changed files 
M       Registry/registry.noahmp
M       phys/noahmp

TESTS CONDUCTED: 
1. The bug fixes have been tested for NoahMP run over the U.S. for an entire year.
2. Reg tests have passed.

RELEASE NOTE: This PR fixes a few variable declaration in subroutine TWOSTREAM from OUT to INOUT, changes soil ice treatment in subroutine COMBINE, and corrects units for three accumulated flux variables in comments and registry. It also removed hard-coded 4 soil layers, and replaces it with parameter NSOIL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants