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

Move the variational scripts to this repository #2920

Merged
merged 37 commits into from
Sep 26, 2024

Conversation

guillaumevernieres
Copy link
Contributor

@guillaumevernieres guillaumevernieres commented Sep 13, 2024

Description

This PR "moves and refactors" the variational DA exscripts that were in the GDASapp to this repository. The ens. var. feature will be replicated/moved in a subsequent PR.

Waiting for 2 companion PR's:

  1. Move the variational scripts to the g-w GDASApp#1282
  2. Revive the marine jcb implementation jcb-gdas#26

ci testing: We should be able to turn on the WCDA test on Hercules

Type of change

  • 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/NO (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?

The latest tests were done on Hecules, earlier ones on 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
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • I have made corresponding changes to the documentation if necessary

ush/python/pygfs/task/marine_bmat.py Outdated Show resolved Hide resolved
ci/cases/gfsv17/ocnanal.yaml Show resolved Hide resolved
jobs/JGLOBAL_MARINE_ANALYSIS_FINALIZE Outdated Show resolved Hide resolved
jobs/JGLOBAL_MARINE_ANALYSIS_FINALIZE Show resolved Hide resolved
jobs/JGLOBAL_MARINE_ANALYSIS_CHECKPOINT Show resolved Hide resolved
ush/python/pygfs/task/marine_bmat.py Outdated Show resolved Hide resolved
ush/python/pygfs/task/marine_bmat.py Outdated Show resolved Hide resolved
ush/python/pygfs/task/marine_bmat.py Outdated Show resolved Hide resolved
ush/python/pygfs/utils/marine_da_utils.py Show resolved Hide resolved
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 to me. No comments beyond what others have already said.

…al_obs_path

* origin/develop:
  Update global atmos upp job to use COMIN/COMOUT (NOAA-EMC#2867)
  Update config.resources for bufr sounding job postsnd (NOAA-EMC#2917)
  Cleanup job for GEFS (NOAA-EMC#2919)
  Build GDASApp and unset memory in Gaea-C5 xml files (NOAA-EMC#2912)
  add 1 deg ocean/ice info to parm/config/gfs/config.resources (NOAA-EMC#2922)
  Support gefs C48 on Azure (NOAA-EMC#2881)
  Disable native grid writes for non-JEDI experiments; update C384 compression options (NOAA-EMC#2914)
hist_date = dparser.parse(ncf.variables['time'].units, fuzzy=True) + timedelta(hours=int(ncf.variables['time'][0]))
ncf.close()
logger.info(f"*** history file date: {hist_date} expected date: {ref_date}")
assert hist_date == ref_date, 'Inconsistent bkg date'
Copy link
Contributor

Choose a reason for hiding this comment

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

raise ValueError if there is a mismatch. assert suggested use is in tests and should not be used in production code.
Use FATAL ERROR language when raising errors.

Comment on lines 138 to 139
f = open(yaml_name, 'w')
yaml.dump(bkg_list, f, sort_keys=False, default_flow_style=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use save_as_yaml

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

I am still combing through this PR and commenting as find time and concrete comments. Apologies for the time, this is a big PR

Comment on lines 281 to 294
case "${machine}" in
"wcoss2") EXPOBS_DIR="" ;;
"hera") EXPOBS_DIR="/scratch1/NCEPDEV/global/glopara/data/experimental_obs" ;;
"orion") EXPOBS_DIR="/work/noaa/global/glopara/data/experimental_obs" ;;
"hercules") EXPOBS_DIR="/work/noaa/global/glopara/data/experimental_obs" ;;
"jet") EXPOBS_DIR="" ;;
"s4") EXPOBS_DIR="" ;;
"gaea") EXPOBS_DIR="" ;;
"noaacloud") EXPOBS_DIR="" ;;
*)
echo "FATAL: Unknown target machine ${machine}, couldn't set EXPOBS_DIR"
exit 1
;;
esac
Copy link
Member

Choose a reason for hiding this comment

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

I have a fork branch with similar but differently done updates to handle the path to the experimental_obs folder. Please see the following diff and consider including my changes instead of the update you have in sorc/link_workflow.sh and DMPDIR in parm/config/gfs/yaml/defaults.yaml:
develop...KateFriedman-NOAA:global-workflow:feature/experimental_obs_path
I'm happy to send a PR into the branch for this PR if you are good with the updates. My updates will resolve #2683.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A pr into my branch sounds good to me @KateFriedman-NOAA , thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Great, please see guillaumevernieres#8. Thanks!

Comment on lines 214 to 215
with open('obs_list_short.yaml', 'w') as file:
yaml.dump(parse_obs_list_file(self.task_config.MARINE_OBS_LIST_YAML), file, default_flow_style=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer using the wxflow save_as_yaml method. See, e.g.

save_as_yaml(self.jedi.config, self.jedi.yaml)

…nto feature/marinevar

* origin/feature/experimental_obs_path:
  Introduce BASE_DATA variable
@guillaumevernieres
Copy link
Contributor Author

Thanks for the recent reviews @aerorahul , @DavidHuber-NOAA and @KateFriedman-NOAA . The comments that I have not addressed in this PR are linked to github issues.
I retested manually on my side but the g-w ci probably needs to be triggered again.

@RussTreadon-NOAA
Copy link
Contributor

Hera g-w CI

Install guillaumevernieres:feature/marinevar at 8002a06 on Hera in /scratch1/NCEPDEV/da/Russ.Treadon/git/global-workflow/pr2920.

Run g-w CI for the following configurations with the following results:

C96C48_hybatmDA

Hera(hfe02):/scratch1/NCEPDEV/stmp2/Russ.Treadon/EXPDIR/prgsi$ rocotostat -d prgsi.db -w prgsi.xml -c all -s
   CYCLE         STATE           ACTIVATED              DEACTIVATED
202112201800        Done    Sep 25 2024 13:28:31    Sep 25 2024 14:20:18
202112210000        Done    Sep 25 2024 13:28:31    Sep 25 2024 22:10:22
202112210600        Done    Sep 25 2024 13:28:31    Sep 25 2024 22:50:18

C96C48_ufs_hybatmDA

Hera(hfe02):/scratch1/NCEPDEV/stmp2/Russ.Treadon/EXPDIR/prjedi$ rocotostat -d prjedi.db -w prjedi.xml -c all -s
   CYCLE         STATE           ACTIVATED              DEACTIVATED
202402231800        Done    Sep 25 2024 13:28:41    Sep 25 2024 14:25:24
202402240000        Done    Sep 25 2024 13:28:41    Sep 26 2024 00:00:56

C96_atmaerosnowDA

Hera(hfe02):/scratch1/NCEPDEV/stmp2/Russ.Treadon/EXPDIR/praero$ rocotostat -d praero.db -w praero.xml -c all -s
   CYCLE         STATE           ACTIVATED              DEACTIVATED
202112201200        Done    Sep 25 2024 13:28:22    Sep 25 2024 14:25:20
202112201800        Done    Sep 25 2024 13:28:22    Sep 25 2024 20:00:28
202112210000        Done    Sep 25 2024 13:28:22    Sep 26 2024 00:25:21

C48mx500_3DVarAOWCDA

Hera(hfe02):/scratch1/NCEPDEV/stmp2/Russ.Treadon/EXPDIR/prwcda$ rocotostat -d prwcda.db -w prwcda.xml -c all -s
   CYCLE         STATE           ACTIVATED              DEACTIVATED
202103241200        Done    Sep 25 2024 13:28:52    Sep 25 2024 14:25:22
202103241800        Done    Sep 25 2024 13:28:52    Sep 25 2024 19:50:34

All jobs successfully run to completion in each of the four tested g-w CI configurations.

@RussTreadon-NOAA
Copy link
Contributor

@WalterKolczynski-NOAA : When can this PR be merged into develop?

Once this PR is merged into develop, PR #2875 can be updated with the post #2920 g-w develop and PR #2875 g-w CI will pass.

Alternatively, PR #2875 can be into develop before PR #2920, but we need to accept the fact that C48mx500_3DVarAOWCDA will fail until PR #2920 is merged into develop.

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.

g-w CI successfully tested in several configurations on Hera (see results here).

Approve.

@aerorahul
Copy link
Contributor

Since we have approval from @RussTreadon-NOAA (running tests and validating that they run to completion successfully), and issues have been opened to address reviewer comments in this PR to be addressed later, we can merge this PR.

@aerorahul aerorahul dismissed their stale review September 26, 2024 15:03

issues have been opened to address reviewer comments to be addressed at a future time.

@aerorahul aerorahul merged commit a694cf1 into NOAA-EMC:develop Sep 26, 2024
5 checks passed
@guillaumevernieres guillaumevernieres deleted the feature/marinevar branch October 3, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-Hera-Passed **Bot use only** CI testing on Hera for this PR has completed successfully CI-Hercules-Passed **Bot use only** CI testing on Hercules for this PR has completed successfully CI-Wcoss2-Passed **Bot use only** CI testing on WCOSS for this PR has completed successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hera specific paths in parm/config/gfs/yaml/defaults.yaml
10 participants