Skip to content

A temporary solution to fix erroneous values of LAI from noah-mp#116

Closed
KaiWang-NOAA wants to merge 1 commit into
NOAA-EMC:developfrom
KaiWang-NOAA:Landusefix_CCPPv17
Closed

A temporary solution to fix erroneous values of LAI from noah-mp#116
KaiWang-NOAA wants to merge 1 commit into
NOAA-EMC:developfrom
KaiWang-NOAA:Landusefix_CCPPv17

Conversation

@KaiWang-NOAA
Copy link
Copy Markdown
Collaborator

@KaiWang-NOAA KaiWang-NOAA commented Jun 11, 2025

PR Checklist

  • This PR has been tested on an RDHPCS machine and/or WCOSS2. Please select below:

    • RDHPCS.
    • WCOSS2.

  • This PR has been tested with the ufs-srweather-app workflow online-cmaq branch.

  • New or updated input data is required by this PR.

  • Baselines are expected to change.

Description

This PR is intended to fix erroneous values of LAI coming from Noah-MP LSM (over snow/ice land use types) that breaks the AQM for some grid cells in the NA domain when testing the GFSv17 CCPP package with Noah-MP LSM for AQM

Issue(s) addressed

https://github.com//issues/115

Dependencies

@KaiWang-NOAA
A temporary solution to fix erroneous values of LAI from noah-mp
ff6f39a 
Fix erroneous values of LAI coming from Noah-MP LSM (over snow/ice land use types) that breaks the AQM for some grid cells in the NA domain when testing the GFSv17 CCPP package with Noah-MP LSM for AQM
@KaiWang-NOAA KaiWang-NOAA mentioned this pull request Jun 11, 2025
Open
@yangfanglin
Copy link
Copy Markdown

yangfanglin commented Jun 11, 2025

There is a recent fix of NOAH-MP over glaciers in the ufs-weather-model, ufs-community/ufs-weather-model#2723. Have you tried the latest ufs-weather-model GFSv17 physics package.

@drnimbusrain
Copy link
Copy Markdown
Contributor

drnimbusrain commented Jun 12, 2025 •
edited
Loading

@KaiWang-NOAA This is good because we need a branch though with only Noah-MP changes for GFSv17 as we will likely only want to test this change AQMv8_p1.1, and isolate the impacts of GFSv17 met during the four months compared to our AQMv8p1 before possibly moving to AQMv8_p2 (with other changes), if that makes sense.

On that note, seems you make a branch from your own repo for https://github.com/KaiWang-NOAA/AQM/tree/Landusefix_CCPPv17, which technically we can test just fine, but wonder why you don't just make a new branch in EMC authoritative repo? I assume you have the permissions to do so as maintainer or admin here, correct? We rather use authoritative repos only for prototypes.

I think maybe its best to do it all in the EMC auth repo and we can test the branch and track using these prototype runs before merging into final [develop] branch.

@drnimbusrain
Copy link
Copy Markdown
Contributor

drnimbusrain commented Jun 12, 2025 •
edited
Loading

There is a recent fix of NOAH-MP over glaciers in the ufs-weather-model, ufs-community/ufs-weather-model#2723. Have you tried the latest ufs-weather-model GFSv17 physics package.

@yangfanglin @KaiWang-NOAA I also think this could be a good idea as we communicated at the time with Mike Barlarge (via emails) about this issue with Noah-MP erroneous LAI values over glacial points. We may need a test without the fix first to see if all is OK now with these erroneous LAI values in latest UWM Noah-MP version. However, @yangfanglin that PR ufs-community/ufs-weather-model#2723 doesn't seem to deal with erroneous LAI values here, but rather canopy liquid water.

@drnimbusrain
Copy link
Copy Markdown
Contributor

drnimbusrain commented Jun 12, 2025

@KaiWang-NOAA This is good because we need a branch though with only Noah-MP changes for GFSv17 as we will likely only want to test this change AQMv8_p1.1, and isolate the impacts of GFSv17 met during the four months compared to our AQMv8p1 before possibly moving to AQMv8_p2 (with other changes), if that makes sense.

On that note, seems you make a branch from your own repo for https://github.com/KaiWang-NOAA/AQM/tree/Landusefix_CCPPv17, which technically we can test just fine, but wonder why you don't just make a new branch in EMC authoritative repo? I assume you have the permissions to do so as maintainer or admin here, correct? We rather use authoritative repos only for prototypes.

I think maybe its best to do it all in the EMC auth repo and we can test the branch and track using these prototype runs before merging into final [develop] branch.

Its OK, understand only Brian has those permissions to make branches here. We can test this fork/branch in AQMv8_p1.1.

@BrianCurtis-NOAA
Copy link
Copy Markdown
Collaborator

BrianCurtis-NOAA commented Jun 12, 2025

@drnimbusrain There's no reason to create personal branches in an authoritative repo. If multiple people wish to work together on something, then you can all work from one persons fork/branch together (make PR's to that persons fork/branch for updates/changes). Then the final PR to the auth repo will come from that one persons fork/branch.

@drnimbusrain
Copy link
Copy Markdown
Contributor

drnimbusrain commented Jun 12, 2025 via email

bbakernoaa
bbakernoaa reviewed Jun 13, 2025
View reviewed changes
Comment thread src/shr/aqm_methods.F90
do r = row0, row1
do c = col0, col1
k = k + 1
if ( stateIn % xlai(c,r) > 10.0 ) then !zero out erroneously high values
Copy link
Copy Markdown
Collaborator

@bbakernoaa bbakernoaa Jun 13, 2025

Choose a reason for hiding this comment

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

should probably add something here to ensure positive. this seems like a good idea regardless if the latest UWM fixed this. always good to ensure valid values.

@drnimbusrain
Copy link
Copy Markdown
Contributor

drnimbusrain commented Jun 16, 2025 •
edited
Loading

@KaiWang-NOAA I have tested our AQMv8_p1.1 branch based on recent SRW-App [develop] on C6 (and current AQM [develop]), and the run completes OK without this fix in. Doesn't seem like it is needed anymore. Is your test consistent?

24-hour outputs: /gpfs/f6/bil-fire3/scratch/Patrick.C.Campbell/expt_dirs/aqmv8p1.1_testbed_AQMNA13km_warmstart/2023040212

@KaiWang-NOAA
Copy link
Copy Markdown
Collaborator Author

KaiWang-NOAA commented Jun 16, 2025 via email

@KaiWang-NOAA
Copy link
Copy Markdown
Collaborator Author

KaiWang-NOAA commented Jun 16, 2025 via email

@KaiWang-NOAA
Copy link
Copy Markdown
Collaborator Author

KaiWang-NOAA commented Jun 17, 2025

I'm closing PR since the new test run with recent updated Noah-MP LSM in UFSWM (ufs-community/ufs-weather-model#2723) has fix the issue.

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

Reviewers

@bbakernoaa bbakernoaa bbakernoaa left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@KaiWang-NOAA @yangfanglin @drnimbusrain @BrianCurtis-NOAA @bbakernoaa