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

[uwtools_integration] Integrate ics/lbcs #266

Open
wants to merge 44 commits into
base: uwtools_integration
Choose a base branch
from

Conversation

WeirAE
Copy link
Collaborator

@WeirAE WeirAE commented Sep 19, 2024

DESCRIPTION OF CHANGES:

A full integration of uwtools for the make_ics and make_lbcs tasks.

All fundamental tests have passed

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

TESTS CONDUCTED:

  • hera.intel
  • fundamental test suite

@WeirAE WeirAE self-assigned this Sep 19, 2024
@WeirAE
Copy link
Collaborator Author

WeirAE commented Oct 24, 2024

This will need to be updated with the changes from #268, but it is sufficiently complete otherwise for feedback

Copy link

@NaureenBharwaniNOAA NaureenBharwaniNOAA left a comment

Choose a reason for hiding this comment

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

Here's my initial review. Will review more of it soon!

parm/wflow/coldstart.yaml Outdated Show resolved Hide resolved
parm/wflow/coldstart.yaml Outdated Show resolved Hide resolved
scripts/chgres_cube.py Outdated Show resolved Hide resolved
Co-authored-by: NaureenBharwaniNOAA <[email protected]>
scripts/chgres_cube.py Outdated Show resolved Hide resolved
scripts/chgres_cube.py Show resolved Hide resolved
scripts/chgres_cube.py Outdated Show resolved Hide resolved
scripts/chgres_cube.py Outdated Show resolved Hide resolved
scripts/chgres_cube.py Show resolved Hide resolved
ush/ccpp_suites_defaults.yaml Outdated Show resolved Hide resolved
ush/config_defaults.yaml Outdated Show resolved Hide resolved
ush/config_defaults.yaml Show resolved Hide resolved
ush/config_defaults.yaml Outdated Show resolved Hide resolved
ush/config_defaults_aqm.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@maddenp-noaa maddenp-noaa left a comment

Choose a reason for hiding this comment

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

I think there might be some changes coming, and will re-review after those. Maybe some changes suggested in this batch of comments will appear, too.

scripts/chgres_cube.py Show resolved Hide resolved
parm/wflow/coldstart.yaml Outdated Show resolved Hide resolved
parm/wflow/coldstart.yaml Outdated Show resolved Hide resolved
return parser.parse_args(argv)


# pylint: disable=too-many-locals, too-many-statements, too-many-branches
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# pylint: disable=too-many-locals, too-many-statements, too-many-branches
# pylint: disable-next=too-many-locals, too-many-statements, too-many-branches

I'm not sure this will work -- whether disable-next will apply to the next block and not just the next line, for those suppressors that relate to this function. If not, this line should probably move to the top of the module (after the #!) because, without -next, it applies to every line of code until the end of the module, and I think we should make this kind of interference with the linter more visibly obvious.

But some of this is also not bad advice from the linter: This function in 177 lines long. Could it be refactored into smaller, purpose-specific functions for better readability? I see what look like section comments describing various (maybe) unrelated operations, each of which could be its own function, which would then be somewhat self-documenting, e.g. we could change

# Extract driver config from experiment config

to function call

driver = get_driver(expt_config_cp)

and factor out the code to that function. If nothing else, there is a giant if / else block where the two alternatives might better be encapsulated in two functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm leaving this open for now to see what you think of the current shorter version. It might be a cleaner version as well if I moved the file linking or the loop off to a helper function?

scripts/chgres_cube.py Outdated Show resolved Hide resolved
ush/config_defaults.yaml Outdated Show resolved Hide resolved
@christinaholtNOAA
Copy link
Collaborator

Hi all, let's hold off on reviews for the moment. I'm going to push some changes. We'll let you know when it's ready for the next round of review.

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.

4 participants