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 sfs as valid system #3243

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

Conversation

WalterKolczynski-NOAA
Copy link
Contributor

@WalterKolczynski-NOAA WalterKolczynski-NOAA commented Jan 21, 2025

Description

Adds sfs as a valid option for NET.

To start, the GEFS system is generally just copied wholesale for SFS. This includes the extract_vars job.

Other than base and resources, config files link to the GEFS versions, just as GEFS config files point to the GFS versions except where they have needed to be changed.

The temporary SFS_POST option has been removed.

The existing SFS test is copied and slightly modified for a PR-level CI test.

Resolves #2271

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? YES
  • Does this change require an update to any of the following submodules? NO

How has this been tested?

  • gfs cycled test on Hercules
  • gefs test on Hercules

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

@WalterKolczynski-NOAA
Copy link
Contributor Author

@NeilBarton-NOAA would you please supply ICs for the SFS test so we can set it up to run on all systems

@WalterKolczynski-NOAA
Copy link
Contributor Author

Still need to add SFS to the RTD documentation.

@WalterKolczynski-NOAA WalterKolczynski-NOAA marked this pull request as draft January 21, 2025 19:19
@aerorahul aerorahul requested a review from pjpegion January 21, 2025 19:26
@WalterKolczynski-NOAA WalterKolczynski-NOAA marked this pull request as ready for review January 23, 2025 03:22
@WalterKolczynski-NOAA
Copy link
Contributor Author

ICs are in place and documentation has been updated. Ready for review.

Adds sfs as a valid option for NET.

To start, the GEFS system is generally just copied wholesale for SFS.
This includes the extract_vars job.

Other than base and resources, config files link to the GEFS versions,
just as GEFS config files point to the GFS versions except where they
have needed to be changed.

The temporarily SFS_POST option has been removed.

The sfs test case is moved from a separate sfs directory to the pr
directory, but ICs are still needed before it is functional.

Resolves NOAA-EMC#2271
The SFS case is moved back to its original position in an sfs directory,
then a copy is made in the PR directory using a shorter time period and
a couple other changes.
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA left a comment

Choose a reason for hiding this comment

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

Here are a couple of suggestions.

tasks += ['wavepostpnt']

if options['do_extractvars']:
tasks += ['extractvars', 'arch']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tasks += ['extractvars', 'arch']
tasks += ['extractvars']

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the arch task be created even when do_extractvars is false? This change has recently been applied to gefs.py.

if options['do_extractvars']:
tasks += ['extractvars', 'arch']

tasks += ['cleanup']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tasks += ['cleanup']
tasks += ['arch', 'cleanup']

Copy link
Contributor

Choose a reason for hiding this comment

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

If the arch task were to be added unconditionally, will a sfs_arcdir.yaml.j2 and master_sfs.yaml.j2 need to be created for the archive task to succeed?

max_tasks = self._configs[config]['MAX_TASKS']
resources = self.get_resource(config)

fhrs = self._get_forecast_hours('gefs', self._configs[config], component)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fhrs = self._get_forecast_hours('gefs', self._configs[config], component)
fhrs = self._get_forecast_hours('sfs', self._configs[config], component)


dependencies = rocoto.create_dependency(dep_condition='and', dep=deps)

fhrs = self._get_forecast_hours('gefs', self._configs['atmos_ensstat'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fhrs = self._get_forecast_hours('gefs', self._configs['atmos_ensstat'])
fhrs = self._get_forecast_hours('sfs', self._configs['atmos_ensstat'])

deps.append(rocoto.add_dependency(dep_dict))
dependencies = rocoto.create_dependency(dep=deps, dep_condition='or')

fhrs = self._get_forecast_hours('gefs', self._configs['wavepostsbs'], 'wave')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fhrs = self._get_forecast_hours('gefs', self._configs['wavepostsbs'], 'wave')
fhrs = self._get_forecast_hours('sfs', self._configs['wavepostsbs'], 'wave')


# Resolution specific parameters
export LEVS=128
export CASE="@CASECTL@" # CASE is required in GEFS to determine ocean/ice/wave resolutions
Copy link
Contributor

Choose a reason for hiding this comment

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

CASE is also required for SFS, right?

Suggested change
export CASE="@CASECTL@" # CASE is required in GEFS to determine ocean/ice/wave resolutions
export CASE="@CASECTL@" # CASE is required in SFS to determine ocean/ice/wave resolutions


# Commonly defined parameters in JJOBS
export envir=${envir:-"prod"}
export NET="sfs" # NET is defined in the job-card (ecf)
Copy link
Contributor

@EricSinsky-NOAA EricSinsky-NOAA Jan 28, 2025

Choose a reason for hiding this comment

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

Since NET is set to sfs, archive.py might fail at lines 89-90 if the sfs yaml does not exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SFS configuration
2 participants