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.

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

Add capability to run diff resolutions for marine anl and background #3238

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

guillaumevernieres
Copy link
Contributor

@guillaumevernieres guillaumevernieres commented Jan 16, 2025

Description

This PR mostly includes configuration changes to allow the use a B-matrix at a defined resolution that may or may not be the same as the background.

Changes of note:

Dependencies

Type of change

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

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO
  • Does this change require an update to any of the following submodules? YES (If YES, please add a link to any PRs that are pending.)
    • EMC verif-global
    • GDAS
    • GFS-utils
    • GSI
    • GSI-monitor
    • GSI-utils
    • UFS-utils
    • UFS-weather-model
    • wxflow

How has this been tested?

Subset of the ci tests on hercules and hera

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

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.

Looks good except for the one comment

sorc/link_workflow.sh Outdated Show resolved Hide resolved
@guillaumevernieres guillaumevernieres added the JEDI Feature development to support JEDI-based DA label Jan 23, 2025
DavidNew-NOAA
DavidNew-NOAA previously approved these changes Jan 23, 2025
@guillaumevernieres
Copy link
Contributor Author

The subset of tests relevant to this PR passed. I'm marking this as ready to review.

@@ -8,12 +8,13 @@ arguments:
resdetatmos: 384
resdetocean: 0.25
nens: 0
interval: 6
interval: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Refresh my memory - what does setting to 0 do to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the gdas cycle will run, not the gfs.

start: warm
comroot: {{ 'RUNTESTS' | getenv }}/COMROOT
expdir: {{ 'RUNTESTS' | getenv }}/EXPDIR
idate: 2021063018
edate: 2021070306
#icsdir: /scratch1/NCEPDEV/climate/Jessica.Meixner/cycling/IC_2021063000_V2
icsdir: /work/noaa/da/gvernier/ensda/ictest/1440x1080x75/
#icsdir: /work/noaa/da/gvernier/ensda/ictest/1440x1080x75/
icsdir: /scratch2/NCEPDEV/ocean/Guillaume.Vernieres/data/prepics/hybrid-test/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

@@ -15,5 +15,6 @@ arguments:
expdir: {{ 'RUNTESTS' | getenv }}/EXPDIR
idate: 2021063018
edate: 2021070306
icsdir: /work/noaa/da/gvernier/ensda/ictest/1440x1080x75/
#icsdir: /work/noaa/da/gvernier/ensda/ictest/1440x1080x75/
icsdir: /scratch2/NCEPDEV/ocean/Guillaume.Vernieres/data/prepics/hybrid-test/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Implements the possibility to run different resolutions for the marine analysis and background Implement capability to run different resolutions for the marine analysis and background Jan 30, 2025
@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Implement capability to run different resolutions for the marine analysis and background Add capability to run different resolutions for the marine analysis and background Jan 30, 2025
@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Add capability to run different resolutions for the marine analysis and background Add capability to run different resolutions for marine analysis and background Jan 30, 2025
@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Add capability to run different resolutions for marine analysis and background Add capability to run diff resolutions for marine analysis and bgrb Jan 30, 2025
@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Add capability to run diff resolutions for marine analysis and bgrb Add capability to run diff resolutions for marine analysis and background Jan 30, 2025
@WalterKolczynski-NOAA WalterKolczynski-NOAA marked this pull request as draft January 30, 2025 15:27
@WalterKolczynski-NOAA WalterKolczynski-NOAA changed the title Add capability to run diff resolutions for marine analysis and background Add capability to run diff resolutions for marine anl and background Jan 30, 2025
@WalterKolczynski-NOAA
Copy link
Contributor

Converting to draft until this is ready.

@guillaumevernieres guillaumevernieres marked this pull request as ready for review January 30, 2025 16:02
@guillaumevernieres
Copy link
Contributor Author

Converting to draft until this is ready.

It was a draft, I converted it to ready to review. If you convert it back to draft, at the very least explain why.

@WalterKolczynski-NOAA
Copy link
Contributor

It was a draft, I converted it to ready to review. If you convert it back to draft, at the very least explain why.

  • There's no description, just a "TODO"
  • Says there is an associated gdas PR but doesn't list it, so I have no idea whether it was merged (and the gdas issue that is listed is still open and also has no PR connected to it). There is a gdas.cd update, so maybe that is all done, but I have no way of knowing that.
  • There are still personal paths in the PR
  • Checklist is un-checked

Those all say to me this PR is not ready. If it is ready, those things need to be addressed.

@guillaumevernieres
Copy link
Contributor Author

It was a draft, I converted it to ready to review. If you convert it back to draft, at the very least explain why.

  • There's no description, just a "TODO"
  • Says there is an associated gdas PR but doesn't list it, so I have no idea whether it was merged (and the gdas issue that is listed is still open and also has no PR connected to it). There is a gdas.cd update, so maybe that is all done, but I have no way of knowing that.
  • There are still personal paths in the PR
  • Checklist is un-checked

Those all say to me this PR is not ready. If it is ready, those things need to be addressed.

Fair enough. I think I made the update to the PR description. @JessicaMeixner-NOAA @CatherineThomas-NOAA and myself added the gfsv17 yaml configurations a few months ago to facilitate running experiments. These have always been pointing to user's path. I can move the offending yamls out this repo if you don't think they should be there.

@WalterKolczynski-NOAA
Copy link
Contributor

Fair enough. I think I made the update to the PR description. @JessicaMeixner-NOAA @CatherineThomas-NOAA and myself added the gfsv17 yaml configurations a few months ago to facilitate running experiments. These have always been pointing to user's path. I can move the offending yamls out this repo if you don't think they should be there.

As discussed in-person before I got a chance to reply, they can stay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JEDI Feature development to support JEDI-based DA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the marine analysis at a resolution different from the deterministic Background error at 1/2 deg
5 participants