Skip to content

apply updates to Gdapapp/land-jediincr to limit max snow increments #3645

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
Open
wants to merge 26 commits into
base: develop
Choose a base branch
from
Open

apply updates to Gdapapp/land-jediincr to limit max snow increments #3645

wants to merge 26 commits into from
+20 −0

Conversation

tsga
Copy link

@tsga tsga commented May 5, 2025 •
edited
Loading

Description

This PR updates GDASApp/land-jediincr to limit positive snow depth increments when background SND exceeds a threshod value. Also enables running the add increment code on more than 6 procs.

Resolves PSL#8
Refs NOAA-EMC/GDASApp#1658
Refs NOAA-EMC/GDASApp#1709

Type of change

  • Bug fix (fixes something broken)
  • New feature (adds functionality)
  • Maintenance (code refactor, clean-up, new CI test, etc.)

Change characteristics

How has this been tested?

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have documented my code, including function, input, and output descriptions
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • This change is covered by an existing CI test or a new one has been added
  • Any new scripts have been added to the .github/CODEOWNERS file with owners
  • I have made corresponding changes to the system documentation if necessary

@tsga tsga mentioned this pull request May 5, 2025
Merged
6 tasks
@tsga tsga marked this pull request as ready for review May 5, 2025 15:33
@aerorahul aerorahul requested a review from DavidNew-NOAA May 12, 2025 18:44
DavidNew-NOAA
DavidNew-NOAA previously approved these changes May 12, 2025
View reviewed changes
Copy link
Contributor

@DavidNew-NOAA DavidNew-NOAA left a comment

Choose a reason for hiding this comment

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

NOAA-EMC/GDASApp#1658 passed CI, so no objections

@CatherineThomas-NOAA
Copy link
Contributor

Hi @tsga: It looks like the GDASApp hash got updated in global-workflow with PR #3624. Was that the only change needed to exercise the limiting of the snow increments?

@tsga
Copy link
Author

tsga commented May 15, 2025

@CatherineThomas-NOAA, there is a namelist modification that needs to be added, but I was wondering if we should close this PR and then add the namelist changes in subsequent PR.

@CatherineThomas-NOAA
Copy link
Contributor

@tsga : I think the change can be included in this PR, but could you convert it to "Draft" while you work out the additional changes and retest and all that?

@tsga tsga closed this May 15, 2025
@tsga tsga reopened this May 15, 2025
@tsga tsga marked this pull request as draft May 15, 2025 14:11
@tsga tsga mentioned this pull request May 20, 2025
Merged
6 tasks
@tsga tsga marked this pull request as ready for review May 20, 2025 16:29
@tsga tsga requested a review from jiaruidong2017 as a code owner May 20, 2025 16:29
CoryMartin-NOAA pushed a commit to NOAA-EMC/GDASApp that referenced this pull request May 20, 2025
@tsga
Update land-jediincr namelists (#1702)
2154dde 
# Description

<!-- Enter PR description here. -->
updates the namelist template for GDASApp/land-jediincr, and fills in
default values to the test namelist.

# Companion PRs

<!-- Enter links to any companion PRs here. -->
NOAA-EMC/global-workflow#3645
# Issues

<!-- Enter any issues referenced or resolved by this PR here. Use
keywords "Resolves" or "Refs".
Resolves #1234
Refs #4321
Refs NOAA-EMC/repo#5678
-->

# Automated CI tests to run in Global Workflow
<!-- Which Global Workflow CI tests are required to adequately test this
PR? -->
- [ ] atm_jjob <!-- JEDI atm single cycle DA !-->
- [ ] C96C48_ufs_hybatmDA <!-- JEDI atm cycled DA !-->
- [x] C96C48_hybatmaerosnowDA  <!-- JEDI aero/snow cycled DA !-->
- [ ] C48mx500_3DVarAOWCDA <!-- JEDI low-res marine 3DVar cycled DA !-->
- [ ] C48mx500_hybAOWCDA <!-- JEDI marine hybrid envar cycled DA !-->
- [ ] C96C48_hybatmDA <!-- GSI atm cycled DA !-->
jiaruidong2017
jiaruidong2017 previously approved these changes May 20, 2025
View reviewed changes
Copy link
Contributor

@jiaruidong2017 jiaruidong2017 left a comment

Choose a reason for hiding this comment

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

looks good to me

@tsga
Copy link
Author

tsga commented May 20, 2025

looks good to me

Thank you @jiaruidong2017! @CatherineThomas-NOAA this PR is now ready.

@tsga
Copy link
Author

tsga commented May 20, 2025

@CoryMartin-NOAA

jiaruidong2017
jiaruidong2017 previously approved these changes May 20, 2025
View reviewed changes
@CoryMartin-NOAA
Copy link
Contributor

NOAA-EMC/GDASApp#1704 will need to be coordinated and merged right before this PR is tested (with an associated hash update) because of changes to the templates that will likely result in snow DA CI failures

@tsga tsga marked this pull request as draft May 21, 2025 11:13
RussTreadon-NOAA
RussTreadon-NOAA reviewed May 21, 2025
View reviewed changes
Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

GDASApp ctests inside g-w failed due to missing comma at the end of lines in snow_analysis.py and snowens_analysis.py

@tsga tsga marked this pull request as ready for review May 21, 2025 12:23
@RussTreadon-NOAA
Copy link
Contributor

Hera Test

I combined g-w PR #3642 and this PR (#3645) into a single working directory on Hera. I used GDASApp develop at 2154dde. Tests

  • test_gdasapp_C96C48_hybatmaerosnowDA_gdas_snowanl_202112201800
  • test_gdasapp_C96C48_hybatmaerosnowDA_enkfgdas_esnowanl_202112201800

failed.

A check of the input yaml shows an empty observer space.

  observations:
    obs perturbations: false
    observers: []

We saw this in PR #3642. Is the empty observer related to PR #3642, something in PR #3645, or something in GDASApp (or jcb-gdas)?

@CoryMartin-NOAA CoryMartin-NOAA mentioned this pull request May 21, 2025
Merged
22 tasks
@tsga
Copy link
Author

tsga commented May 21, 2025 •
edited
Loading

Hera Test

I combined g-w PR #3642 and this PR (#3645) into a single working directory on Hera. I used GDASApp develop at 2154dde. Tests

  • test_gdasapp_C96C48_hybatmaerosnowDA_gdas_snowanl_202112201800
  • test_gdasapp_C96C48_hybatmaerosnowDA_enkfgdas_esnowanl_202112201800

failed.

A check of the input yaml shows an empty observer space.

  observations:
    obs perturbations: false
    observers: []

We saw this in PR #3642. Is the empty observer related to PR #3642, something in PR #3645, or something in GDASApp (or jcb-gdas)?

@RussTreadon-NOAA I am not sure if the above error is coming from it but PR #3645 also requires GDASApp PR (NOAA-EMC/GDASApp#1704).

@DavidNew-NOAA
Copy link
Contributor

@RussTreadon-NOAA When you merged these PRs, were there any conflicts in the snow_analysis.py code? The reason I ask is that, recall, the JEDI YAMLs need to be initialized after the observations are staged, which is why I moved the JEDI initialization the the end of the initialize method in the snow_analysis.py code

@tsga tsga mentioned this pull request May 21, 2025
Merged
6 tasks
CoryMartin-NOAA pushed a commit to NOAA-EMC/GDASApp that referenced this pull request May 21, 2025
@tsga
update the add incr namelist for ensembles (#1704)
93d85ec 
# Description

<!-- Enter PR description here. -->
Earlier pull request only addressed the deterministic one. This one
updates both. Also makes ens_size and variable input, and removes "quote
marks" from around Boolean and numeric variables--per suggestion by the
AI-copilot reviews.
# Companion PRs

<!-- Enter links to any companion PRs here. -->
NOAA-EMC/global-workflow #3645
(NOAA-EMC/global-workflow#3645)
# Issues

<!-- Enter any issues referenced or resolved by this PR here. Use
keywords "Resolves" or "Refs".
Resolves #1234
Refs #4321
Refs NOAA-EMC/repo#5678
-->

# Automated CI tests to run in Global Workflow
<!-- Which Global Workflow CI tests are required to adequately test this
PR? -->
- [ ] atm_jjob <!-- JEDI atm single cycle DA !-->
- [ ] C96C48_ufs_hybatmDA <!-- JEDI atm cycled DA !-->
- [x] C96C48_hybatmaerosnowDA  <!-- JEDI aero/snow cycled DA !-->
- [ ] C48mx500_3DVarAOWCDA <!-- JEDI low-res marine 3DVar cycled DA !-->
- [ ] C48mx500_hybAOWCDA <!-- JEDI marine hybrid envar cycled DA !-->
- [ ] C96C48_hybatmDA <!-- GSI atm cycled DA !-->
@DavidNew-NOAA DavidNew-NOAA mentioned this pull request May 22, 2025
Merged
6 tasks
@CoryMartin-NOAA CoryMartin-NOAA mentioned this pull request May 22, 2025
Open
tsga added 4 commits May 22, 2025 10:50
@tsga
Merge branch 'NOAA-EMC:develop' into feature/lnd_ens
9a33547
@tsga
Merge branch 'develop' into feature/lnd_ens
9c54efc
@tsga
Merge branch 'develop' into feature/lnd_ens
b09d5ae
@tsga
Merge branch 'develop' into feature/lnd_ens
917530d 
* develop:
  Hotfix: Updating ref value in GitHub dispatch action for triggering CTest on GitLab (NOAA-EMC#3728)
  Move default STMP, PTMP, and HOMEDIR paths to world-shared on C6 (NOAA-EMC#3717)
  Update param files for GEFS (NOAA-EMC#3677)
  Update gfs-utils.fd hash to 7361f31a69e9eaa599d708c592e3ac9df3ea1a9e (NOAA-EMC#3727)
  Check if warm restart date is valid (NOAA-EMC#3720)
  Refactor marine analysis Task class (NOAA-EMC#3642)
  GitLab Pipeline updates for CTest Automation against Nightly Runs (NOAA-EMC#3688)
  sfs changes to run on gaeac6
  Check for binary or netCDF WW3 restarts when staging (NOAA-EMC#3719)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussTreadon-NOAA RussTreadon-NOAA RussTreadon-NOAA left review comments

@jiaruidong2017 jiaruidong2017 jiaruidong2017 left review comments

@DavidNew-NOAA DavidNew-NOAA DavidNew-NOAA left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Limit analysis above a maximum snow depth
6 participants
@tsga @CatherineThomas-NOAA @CoryMartin-NOAA @RussTreadon-NOAA @DavidNew-NOAA @jiaruidong2017