Skip to content

In Noah-MP, set zero for canopy liquid and canopy ice over glacier #2723

Merged
FernandoAndrade-NOAA merged 30 commits into
ufs-community:developfrom
wzzheng90:canopywater_glacier
Jun 6, 2025
Merged

In Noah-MP, set zero for canopy liquid and canopy ice over glacier #2723
FernandoAndrade-NOAA merged 30 commits into
ufs-community:developfrom
wzzheng90:canopywater_glacier

Conversation

@wzzheng90
Copy link
Copy Markdown
Contributor

@wzzheng90 wzzheng90 commented May 2, 2025

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

Description:

This PR addresses the issue Fixes NCAR/ccpp-physics#272: ufs-community/ccpp-physics#272.

Failure of some UFS prototypes (e.g., GEFS) results from the "bad" canopy water used by the Noah-MP land surface model. In fact, canopy water is sum of canopy liquid and canopy ice in the Noah-MP, which is available only at the vegetation grids and there is no canopy variable at the glacier grids. Therefore, it doesn't really matter what canopy variables including canopy water are at initial time or at the forecast time over the glacier, so canopy liquid or canopy ice is set with a missing values (9.99e20_kind_phys).

However, some other processes such as UFS cold starts, outputs, restart and CHGRES steps, sometimes do not exclude 'glacier fraction' when processing canopy variables, and assign unrealistically canopy values over non-glacier points.

As one of options, set canopy liquid=0.0 and canopy ice=0.0 in Noah-MP, so that it avoids failure of some UFS prototypes (e.g., GEFS) results from the "bad" canopy water used by the Noah-MP land surface model.

Further updated:
a) Set all the related canopy "undefined" over the glacier to some values;
b) Modified snow glacier calculation in Noah-MP.

Performed some regression tests ( with Gaussian_grid and cubed_sphere_grid, separately ) for two sensitivity runs: a) setting all the "undefined" over glacier to some values, and b) modification of the snow glacier ( "module_sf_noahmp_glacier.F90" ) plus a) changes.
The a) run has no impacts at the non-glacier grids which we expected, while the b) run indicates some small changes of simulation. Several slides (PPT) are attached.

After completing the full Intel+GNU RT suite suite tests, it is found that two RRFS tests failed to run due to the division to zero causing floating point exception: both rrfs_v1beta_debug tests for Intel and GNU
We need to fix these tests later after identifying the causes. We now comment out both rrfs_v1beta_debug tests for Intel and GNU in 'rt.conf', and skip these two tests for now. We'll revisit them later.

Commit Message:

* UFSWM - Set all the related canopy "undefined" over the glacier to some values and modified snow glacier calculation in Noah-MP.
  * FV3 - Set all the related canopy "undefined" over the glacier to some values and modified snow glacier calculation in Noah-MP.
    * ccpp-physics - Set all the related canopy "undefined" over the glacier to some values and modified snow glacier calculation in Noah-MP.
  

Priority:

  • High

Git Tracking

UFSWM:

  • None

Sub component Pull Requests:

UFSWM Blocking Dependencies:

  • None

Documentation:

  • No documentation update is required for this PR (please explain).

Changes

Regression Test Changes (Please commit test_changes.list):

  • PR updates/changes baselines.

Input data Changes:

  • None.

Library Changes/Upgrades:

  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • GaeaC6
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unne

@wzzheng90 wzzheng90 marked this pull request as ready for review May 2, 2025 18:48
@wzzheng90
Copy link
Copy Markdown
Contributor Author

Combined the modification for snow glacier in "module_sf_noahmp_glacier.F90" from Mike Barlage.

@wzzheng90
Copy link
Copy Markdown
Contributor Author

Further updated the glacier variables related to the canopy and set all the undefined over the glacier to some values suggested by Mike Barlage.

Performed some regression tests ( with Gaussian_grid and cubed_sphere_grid, separately ) for two sensitivity runs: a) setting all the "undefined" over glacier to some values, and b) modification of the snow glacier ( "module_sf_noahmp_glacier.F90" ) plus a) changes.
The a) run has no impacts at the non-glacier grids which we expected, while the b) run indicates some small changes of simulation. Several slides (PPT) are attached.

@grantfirl
Copy link
Copy Markdown
Collaborator

@wzzheng90 It looks like you deleted the pull request template when you put in your description. UFS code managers need to see/know the information contained in the template. Can you please put the template information back, fill it out, and only paste your description in the "Description" section of the template? Same goes for NOAA-EMC/ufsatm#960. The pull request templates are very important for the code managers.

@rhaesung
Copy link
Copy Markdown
Contributor

@jkbk2004 @grantfirl @JessicaMeixner-NOAA @RuiyuSun This PR is ready.

@grantfirl
Copy link
Copy Markdown
Collaborator

@jkbk2004 @grantfirl @JessicaMeixner-NOAA @RuiyuSun This PR is ready.

Thanks @rhaesung @wzzheng90 I'm assuming that there will be an associated PR into https://github.com/NOAA-EMC/noahmp?

Also, are all the failed tests in test_changes.list expected? It looks like every test that uses NOAH-mp should have different results, correct?

@wzzheng90
Copy link
Copy Markdown
Contributor Author

@grantfirl Yes, these changes in Noah-MP should have different results.

@rhaesung
Copy link
Copy Markdown
Contributor

@jkbk2004 @grantfirl @JessicaMeixner-NOAA @RuiyuSun This PR is ready.

Thanks @rhaesung @wzzheng90 I'm assuming that there will be an associated PR into https://github.com/NOAA-EMC/noahmp?

Also, are all the failed tests in test_changes.list expected? It looks like every test that uses NOAH-mp should have different results, correct?

@grantfirl Yes, we’re aware of this. @wzzheng90, as Grant suggested, we ultimately need an associated PR submitted to https://github.com/NOAA-EMC/noahmp.

@wzzheng90
Copy link
Copy Markdown
Contributor Author

@rhaesung Thanks. Please let me know when we create a PR submitted to https://github.com/NOAA-EMC/noahmp.

@rhaesung
Copy link
Copy Markdown
Contributor

@rhaesung Thanks. Please let me know when we create a PR submitted to https://github.com/NOAA-EMC/noahmp.

Thanks. Please let me know when we create a PR submitted to https://github.com/NOAA-EMC/noahmp.

@wzzheng90 Sure. When you have time, we can go ahead and start working on this PR together.

@grantfirl
Copy link
Copy Markdown
Collaborator

@wzzheng90 @rhaesung I don't think that it is necessarily required to have the noahmp PR attached to this one if there is time pressure, as long as it gets done at some point. That is, unless noahmp is being called as a component in GFS v17 or something.

@rhaesung
Copy link
Copy Markdown
Contributor

@wzzheng90 @rhaesung I don't think that it is necessarily required to have the noahmp PR attached to this one if there is time pressure, as long as it gets done at some point. That is, unless noahmp is being called as a component in GFS v17 or something.

@grantfirl I agree — this isn’t urgent. Thanks for the clarification! @wzzheng90 We can proceed without attaching the NoahMP PR for now, as long as it's addressed later.

@wzzheng90
Copy link
Copy Markdown
Contributor Author

@grantfirl @rhaesung Sounds good. Thanks for your comments.

@rhaesung
Copy link
Copy Markdown
Contributor

Reported failed RRFS tests. Please hold off on this PR for now.

…due to the floating point exception and we will revisit them later
@rhaesung
Copy link
Copy Markdown
Contributor

We've decided to skip the RRFS tests - see #2759 for details. This should now be ready with the newly disabled tests (cc9353c). @yangfanglin @wzzheng90 @barlage @RuiyuSun @JessicaMeixner-NOAA @grantfirl @jkbk2004

@jkbk2004
Copy link
Copy Markdown
Collaborator

@wzzheng90 can you sync up branches?

@wzzheng90
Copy link
Copy Markdown
Contributor Author

@wzzheng90 can you sync up branches?

@jkbk2004 Done. Please check if they are correct. Thanks!

@FernandoAndrade-NOAA
Copy link
Copy Markdown
Collaborator

#2739 has been merged. This PR is next in queue, @wzzheng90 please sync up all branches and resolve conflicts, thank you.

@FernandoAndrade-NOAA
Copy link
Copy Markdown
Collaborator

Noting that hera saw these failures not within test_changes:

cpld_control_gfsv17_nowav_iau intel
cpld_restart_gfsv17_nowav_iau intel
cpld_control_c48_5deg intel
cpld_warmstart_c48_5deg intel
cpld_restart_c48_5deg intel
control_wam_debug gnu
cpld_control_p8 gnu
cpld_debug_p8 gnu
cpld_debug_pdlib_p8 gnu

@wzzheng90 are the updates in this pull request expecting changes to these tests? We can add them to test_changes for logging, but we need verification from your side that these are expected.

@DeniseWorthen
Copy link
Copy Markdown
Collaborator

@FernandoAndrade-NOAA Just to confirm, the additional tests in your list ran to completion, but did not pass baselines?

@wzzheng90
Copy link
Copy Markdown
Contributor Author

Noting that hera saw these failures not within test_changes:

cpld_control_gfsv17_nowav_iau intel
cpld_restart_gfsv17_nowav_iau intel
cpld_control_c48_5deg intel
cpld_warmstart_c48_5deg intel
cpld_restart_c48_5deg intel
control_wam_debug gnu
cpld_control_p8 gnu
cpld_debug_p8 gnu
cpld_debug_pdlib_p8 gnu

@wzzheng90 are the updates in this pull request expecting changes to these tests? We can add them to test_changes for logging, but we need verification from your side that these are expected.

@FernandoAndrade-NOAA I think yes, but I have to double check these tests. Could you please inform me where to find these failures that Hera saw? Which output files?

@FernandoAndrade-NOAA
Copy link
Copy Markdown
Collaborator

@FernandoAndrade-NOAA Just to confirm, the additional tests in your list ran to completion, but did not pass baselines?

Correct, the test runs themselves seem fine.

Noting that hera saw these failures not within test_changes:

cpld_control_gfsv17_nowav_iau intel
cpld_restart_gfsv17_nowav_iau intel
cpld_control_c48_5deg intel
cpld_warmstart_c48_5deg intel
cpld_restart_c48_5deg intel
control_wam_debug gnu
cpld_control_p8 gnu
cpld_debug_p8 gnu
cpld_debug_pdlib_p8 gnu

@wzzheng90 are the updates in this pull request expecting changes to these tests? We can add them to test_changes for logging, but we need verification from your side that these are expected.

@FernandoAndrade-NOAA I think yes, but I have to double check these tests. Could you please inform me where to find these failures that Hera saw? Which output files?

Please refer to /scratch1/NCEPDEV/stmp2/Fernando.Andrade-maldonado/FV3_RT/rt_1126622/ for the test directories for the tests listed.

@wzzheng90
Copy link
Copy Markdown
Contributor Author

@FernandoAndrade-NOAA Just to confirm, the additional tests in your list ran to completion, but did not pass baselines?

Correct, the test runs themselves seem fine.

Noting that hera saw these failures not within test_changes:

cpld_control_gfsv17_nowav_iau intel
cpld_restart_gfsv17_nowav_iau intel
cpld_control_c48_5deg intel
cpld_warmstart_c48_5deg intel
cpld_restart_c48_5deg intel
control_wam_debug gnu
cpld_control_p8 gnu
cpld_debug_p8 gnu
cpld_debug_pdlib_p8 gnu

@wzzheng90 are the updates in this pull request expecting changes to these tests? We can add them to test_changes for logging, but we need verification from your side that these are expected.

@FernandoAndrade-NOAA I think yes, but I have to double check these tests. Could you please inform me where to find these failures that Hera saw? Which output files?

Please refer to /scratch1/NCEPDEV/stmp2/Fernando.Andrade-maldonado/FV3_RT/rt_1126622/ for the test directories for the tests listed.

@FernandoAndrade-NOAA Thank you very much for providing information! I check all runs on the list (except for two runs are not available: cpld_restart_gfsv17_nowav_iau_intel; cpld_restart_c48_5deg_intel), and this PR changes results in these tests.

@DeniseWorthen
Copy link
Copy Markdown
Collaborator

The documentation update has already been merged. Please update the PR title.

@jkbk2004 jkbk2004 changed the title In Noah-MP, set zero for canopy liquid and canopy ice over glacier + Urgent Documentation Update for HSD Capability Announcement #2765 In Noah-MP, set zero for canopy liquid and canopy ice over glacier Jun 5, 2025
@jkbk2004
Copy link
Copy Markdown
Collaborator

jkbk2004 commented Jun 5, 2025

@FernandoAndrade-NOAA
Copy link
Copy Markdown
Collaborator

Ok we should be all set with testing, leaving a note in subcomponents to continue with their merge process. Derecho will be skipped due to system maintenance.

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

Labels

Baseline Updates Current baselines will be updated. Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants