search

wrf-model / WRF

The official repository for the Weather Research and Forecasting (WRF) model
Other
1.18k stars 658 forks source link

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

Open joshi994 opened 3 months ago

joshi994 commented 3 months ago

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 commented 3 months ago
Screenshot 2024-04-01 at 12 30 48 PM

Please refer to the attached.

weiwangncar commented 2 months ago

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 commented 2 months ago

@cenlinhe Can you review this PR? Thanks!

cenlinhe commented 2 months ago

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

weiwangncar commented 2 months ago

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

cenlinhe commented 2 months ago

@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?

joshi994 commented 2 months ago

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

cenlinhe commented 2 months ago

@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 commented 2 months ago

@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!

cenlinhe commented 2 months ago

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

joshi994 commented 2 months ago

@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: https://github.com/wrf-model/WRF/pull/2038

cenlinhe commented 2 months ago

@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 commented 2 months ago

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

joshi994 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Thanks for confirming. Please let me know if you need further clarification.

From: Cenlin_He @.> Date: Wednesday, April 17, 2024 at 6:10 PM To: wrf-model/WRF @.> Cc: Joshi, Parag @.>, Mention @.> Subject: Re: [wrf-model/WRF] Modified/Corrected LECH's stability functions in the SL Urban Canopy Model following the formulation. (PR #2032)

looks like this PR is reverted back and correct now.

— Reply to this email directly, view it on GitHubhttps://github.com/wrf-model/WRF/pull/2032#issuecomment-2062531294, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BGF4AMW34XMKR7QKUWZGJVDY53XMJAVCNFSM6AAAAABFR3BMUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRSGUZTCMRZGQ. You are receiving this because you were mentioned.Message ID: @.***>

cenlinhe commented 2 months ago

@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 commented 2 months ago

@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 (https://github.com/wrf-model/WRF/pull/2038) and that's what caused the issue. Thanks for your response.

jesusff commented 2 months ago

Hi, you might want to take the opportunity to move the misplaced Lech equation to its place, as in my last commit above https://github.com/CORDEX-WRF-community/WRF/commit/1a2492c3647fc9612e47e4f185c7e6c0f05cf4f6

tslin2 commented 2 weeks ago

@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!

yes, the changes is consistent with the papers.