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

Modified/Corrected LECH's stability functions in the SL Urban Canopy Model following the formulation. #2032

Open
wants to merge 1 commit into
base: release-v4.6.0
Choose a base branch
from
Open

Modified/Corrected LECH's stability functions in the SL Urban Canopy Model following the formulation. #2032

wants to merge 1 commit into from

Conversation

joshi994
Copy link
Contributor

@joshi994 joshi994 commented Apr 1, 2024 •
edited by weiwangncar

TYPE: Bug fix

KEYWORDS: LECH stability functions, Latent heat flux, Sensible heat flux, SL Urban Canpy Model

SOURCE: Parag Joshi (Brookhaven National Lab), Katia Lamer (Brookhaven National Lab)

DESCRIPTION OF CHANGES:
Problem:
The stabvility functions originally proposed by Lech Lobocki which are used in calculating the exchange coefficients between surface layer and roof, building wall, Green roof, and ground are implemented incorrectly. The error surfaced while comparing the formulation suggested in Lobocki 1992 with the equations in commands/lines 3056 and 3062 (WRF-v4.5.2).

Solution:
The code has been corrected by referring to the equations 48 and 49 of the article "A procedure for the derivation of surface layer bulk relationahips from simplified second-order closure models" by Lobocki 1992 (Journal of Applied Meteorology).

ISSUE: For use when this PR closes an issue.
Fixes #123

LIST OF MODIFIED FILES:
M phys/module_sf_urban.F

TESTS CONDUCTED:

  1. The significance of the corrections can be highlighted by comparing a test run with the corrected module and current version of the WRF model.
  2. The fix will correct the evaluation of the exchange coefficients between the ground, roof, green roof etc and air.
  3. The Jenkins tests are all passing.

RELEASE NOTE: Lech's stability functions are corrected following the original formulation.

@joshi994
TYPE: Bug fix
12f6f66 
KEYWORDS: LECH stability functions, Latent heat flux, Sensible heat flux, energy balance, Surface temperature, SL Urban Canpy Model

SOURCE: Parag Joshi (Brookhaven National Lab), Katia Lamer (Brookhaven National Lab)

DESCRIPTION OF CHANGES:
Problem:
The stabvility functions originally proposed by Lech Lobocki which are used in calculating the exchange coefficients between surface layer and roof, building wall, Green roof, and ground are implemented incorrectly. The error surfaced while comparing the formulation suggested in Lobocki 1992 with the equations in commands/lines 3056 and 3062 (WRF-v4.5.2).

Solution:
The code has been corrected by referring to the equations 48 and 49 of the article "A procedure for the derivation of surface layer bulk relationahips from simplified second-order closure models" by Lobocki 1992 (Journal of Applied Meteorology).

LIST OF MODIFIED FILES: module_sf_urban.F
@joshi994 joshi994 requested a review from a team as a code owner April 1, 2024 16:28
@joshi994
Copy link
Contributor Author

joshi994 commented Apr 1, 2024

Screenshot 2024-04-01 at 12 30 48 PM

Please refer to the attached.

@weiwangncar weiwangncar changed the base branch from master to develop April 7, 2024 20:09
@weiwangncar
Copy link
Collaborator

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

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

@weiwangncar
Copy link
Collaborator

@cenlinhe Can you review this PR? Thanks!

@cenlinhe
Copy link
Contributor

cenlinhe commented Apr 8, 2024 •
edited

OK, this is indeed a bug and the proposed fix looks correct to me. I guess this error came from the original code of Noah LSM SFCDIF subroutine based on Chen et al., (1997, BLM): https://link.springer.com/article/10.1023/A:1000531001463
which was copied and pasted to the urban part. The paper shows the correct equations (A7 and A8 in the appendix) but the code implementation is wrong.
The current Noah-MP SFCDIF2 subroutine (based on Chen et al., 1997) also has this bug: https://github.com/NCAR/noahmp/blob/noahmp_v4.5_develop/src/module_sf_noahmplsm.F#L4810-L4813
which needs to be fixed too.
@dudhia @barlage

dudhia
dudhia previously approved these changes Apr 11, 2024
View reviewed changes
Copy link
Collaborator

@dudhia dudhia left a comment

Choose a reason for hiding this comment

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

I approve and defer to others on details of this.

@weiwangncar weiwangncar changed the base branch from develop to release-v4.6.0 April 11, 2024 01:44
@weiwangncar weiwangncar dismissed dudhia’s stale review April 11, 2024 01:44

The base branch was changed.

@weiwangncar
Copy link
Collaborator

@cenlinhe Thanks for reviewing this PR. Would you approve this?

@cenlinhe
Copy link
Contributor

@cenlinhe Thanks for reviewing this PR. Would you approve this?

Yes, I approve this. @tslin2 could you please also take a look when you have time?

weiwangncar
weiwangncar previously approved these changes Apr 11, 2024
View reviewed changes
dudhia
dudhia previously approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@dudhia dudhia left a comment

Choose a reason for hiding this comment

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

I approve and defer to others on details of this.

@joshi994
Copy link
Contributor Author

@cenlinhe Should I go ahead an make changes in the MoahMP-SFCDIF2 as well or you are going to update the subroutine?

@cenlinhe
Copy link
Contributor

@cenlinhe Should I go ahead an make changes in the MoahMP-SFCDIF2 as well or you are going to update the subroutine?

No, you do not need to do it. I will fix them later.

@tslin2
Copy link

tslin2 commented Apr 13, 2024

@cenlinhe Thanks for reviewing this PR. Would you approve this?

Yes, I approve this. @tslin2 could you please also take a look when you have time?

I will try to look at this next week!

@joshi994 joshi994 dismissed stale reviews from dudhia and weiwangncar via 45a820b April 17, 2024 18:05
@cenlinhe
Copy link
Contributor

@joshi994 do you have a reference showing the equations for your latest bug fix commit?

@joshi994
Copy link
Contributor Author

joshi994 commented Apr 17, 2024 •
edited

@joshi994 do you have a reference showing the equations for your latest bug fix commit?

@cenlinhe I messed up in my latest commit. I have submitted a different pull request. Please refer to the link:
#2038

@cenlinhe
Copy link
Contributor

@joshi994 do you have a reference showing the equations for your latest bug fix commit?

@cenlinhe I messed up in my latest commit. I have submitted a different pull request. Please refer to the link: #2038

OK, I think you may need to revert back to the original commit by removing the latest one, otherwise this PR seems being messed up.

@cenlinhe
Copy link
Contributor

Your previous changes seems gone. They were replaced by your latest code changes.

@joshi994
Copy link
Contributor Author

Your previous changes seems gone. They were replaced by your latest code changes.

@cenlinhe I am new to GitHub commit. I may need some time to correct the error. What do you suggest?

@cenlinhe
Copy link
Contributor

Your previous changes seems gone. They were replaced by your latest code changes.

@cenlinhe I am new to GitHub commit. I may need some time to correct the error. What do you suggest?

Sounds good. It's OK for you to take some time to correct this error. Thanks.

@joshi994
Copy link
Contributor Author

Your previous changes seems gone. They were replaced by your latest code changes.

@cenlinhe I am new to GitHub commit. I may need some time to correct the error. What do you suggest?

Sounds good. It's OK for you to take some time to correct this error. Thanks.

@cenlinhe I tried reverting the error. Can you please confirm if the changes are restored and the new PR is still there?

@cenlinhe
Copy link
Contributor

cenlinhe commented Apr 17, 2024 •
edited

looks like this PR is reverted back and correct now. the new PR (2038) is also there. When you make any changes to your new PR (2038), please make sure you are working on your "my_changes2" branch instead of "my_changes1" branch. The "my_changes1" branch is for this PR here.

@joshi994
Copy link
Contributor Author

joshi994 commented Apr 17, 2024 via email

@cenlinhe
Copy link
Contributor

@joshi994 just to double check. please see if this is your original PR bug fix code changes: https://github.com/wrf-model/WRF/pull/2032/files
I think it is.

@joshi994
Copy link
Contributor Author

@joshi994 just to double check. please see if this is your original PR bug fix code changes: https://github.com/wrf-model/WRF/pull/2032/files I think it is.

@cenlinhe Yes, this is the original PR bug fix changes. I accidentally hit my_changes1 for the new PR (#2038) and that's what caused the issue. Thanks for your response.

@cenlinhe cenlinhe mentioned this pull request Apr 18, 2024
Merged
jesusff added a commit to CORDEX-WRF-community/WRF that referenced this pull request Apr 19, 2024
@jesusff
Corrected LECH's stability functions in SLUCM (module_sf_urban.F)
225661d 
See CORDEX-WRF-community/fps-urb-rcc#6

Correction scheduled to appear in WRF v4.6. This still needs to be corrected in Noah MP to be complete. See wrf-model#2032
jesusff added a commit to CORDEX-WRF-community/noahmp that referenced this pull request Apr 19, 2024
@jesusff
Corrected LECH's stability functions
e348cac 
See NCAR#123 in the refactored code and wrf-model/WRF#2032

This is paired to CORDEX-WRF-community/WRF@225661d in SLUCM
jesusff added a commit to CORDEX-WRF-community/WRF that referenced this pull request Apr 19, 2024
@jesusff
Move Lech equation out of Paulson section
1a2492c 
No code change, just cleaner since one of Lech Lobocki's equations for heat (PSLHS) was under the Paulson equations (PSP..) section.

See eq. A5-8 in https://doi.org/10.1023/A:1000531001463 and wrf-model#2032
@jesusff
Copy link
Contributor

jesusff commented Apr 19, 2024

Hi, you might want to take the opportunity to move the misplaced Lech equation to its place, as in my last commit above CORDEX-WRF-community@1a2492c

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

@weiwangncar weiwangncar weiwangncar left review comments

@dudhia dudhia dudhia left review comments

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

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@joshi994 @weiwangncar @cenlinhe @tslin2 @jesusff @dudhia