Skip to content

UW-463 UPP driver#481

Merged
maddenp-cu merged 70 commits into
ufs-community:mainfrom
maddenp-cu:uw-463-upp-driver
May 10, 2024
Merged

UW-463 UPP driver#481
maddenp-cu merged 70 commits into
ufs-community:mainfrom
maddenp-cu:uw-463-upp-driver

Conversation

@maddenp-cu
Copy link
Copy Markdown
Collaborator

@maddenp-cu maddenp-cu commented May 6, 2024

Synopsis

A UPP driver.

Most of the changed pages are small doc fixes / updates / harmonizations.

Type

  • Enhancement (adds new functionality)

Impact

  • This is a non-breaking change (existing functionality continues to work as expected)

Checklist

  • I have added myself and any co-authors to the PR's Assignees list.
  • I have reviewed the documentation and have made any updates necessitated by this change.

Comment thread docs/sections/user_guide/cli/drivers/chgres_cube.rst
Comment thread docs/index.rst
Comment thread docs/shared/upp.yaml
Copy link
Copy Markdown
Contributor

@nbharwani-cu nbharwani-cu left a comment

Choose a reason for hiding this comment

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

I appreciate the work to make all drivers consistent with each other. This is great, nice work!

Comment thread docs/sections/user_guide/yaml/components/upp.rst Outdated
Comment thread src/uwtools/api/sfc_climo_gen.py
Comment thread src/uwtools/drivers/upp.py
Comment thread src/uwtools/resources/jsonschema/upp.jsonschema
Comment thread src/uwtools/resources/jsonschema/upp.jsonschema
Comment thread docs/sections/user_guide/yaml/components/upp.rst
Comment thread src/uwtools/tests/test_schemas.py
Copy link
Copy Markdown
Contributor

@WeirAE WeirAE left a comment

Choose a reason for hiding this comment

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

Whew, this covered a ton! I think I checked everything, I'll double-check in the morning if its still pending but I think it looks good!

Copy link
Copy Markdown
Collaborator

@elcarpenterNOAA elcarpenterNOAA left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Copy Markdown
Collaborator

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

This is looking great! With so many changes, it's gotten me thinking about tiny little upgrades that might be helpful. Some are inline below.

One that I didn't put inline...as I was looking through the thorough tests for namelist settings that are tested against update_values, I'm realizing that we don't actually schema-check the final namelist that we dump to the run directory (unless I'm missing it). A user could have total trash in the base_file and we wouldn't help them correct that at all. I am also now curious if we want to put a requirement on the base_file existence before we try to update it. All of these suggestions are definitely contingent on the user providing a base_file entry.

Comment thread docs/sections/user_guide/cli/drivers/upp.rst Outdated
Comment thread docs/shared/upp.yaml
Comment thread docs/shared/upp.yaml Outdated
Comment thread src/uwtools/cli.py
Comment thread src/uwtools/drivers/driver.py
@maddenp-cu
Copy link
Copy Markdown
Collaborator Author

maddenp-cu commented May 8, 2024

@christinaholtNOAA

we don't actually schema-check the final namelist that we dump to the run directory

We might consider the ability to provide an arbitrary namelist as the relief valve for a user who has modified the namelist requirements (and code) in an unanticipated way and wants to do something our schema doesn't permit. Previously, the entire namelist: block was optional, so a user could simply omit it and manually place a namelist file in the run directory. We decided to make namelist: required and use base_file: as the mechanism to supply a pre-made namelist file, so if we added a schema check of the namelist content, we'd revoke the ability of users to do arbitrary experiments. We could consider adding an optional validate: key under namelist:, defaulting to true, to suppress the check. That might preserve the relieve valve while defaulting to rigorous checking. I'll write a ticket for it and we can evaluate it during a grooming session.

I am also now curious if we want to put a requirement on the base_file existence before we try to update it.

Excellent idea. I'll add a ticket.

@maddenp-cu
Copy link
Copy Markdown
Collaborator Author

I made some updates to support --leadtime HH[:MM[:SS]] instead of an integer number of hours. We should still consider whether we've made any assumptions in the overall codebase about leadtime resolution, but at least the current PR should move us in the right direction by supporting subhourly intervals for UPP.

Copy link
Copy Markdown
Collaborator

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@maddenp-cu maddenp-cu merged commit 2d48df3 into ufs-community:main May 10, 2024
@maddenp-cu maddenp-cu deleted the uw-463-upp-driver branch May 10, 2024 01:19
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.

5 participants