Skip to content

split prep_ngen_data into steps - #112

Open
JordanLaserGit wants to merge 11 commits into
mainfrom
refactor
Open

split prep_ngen_data into steps#112
JordanLaserGit wants to merge 11 commits into
mainfrom
refactor

Conversation

@JordanLaserGit

@JordanLaserGit JordanLaserGit commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

WIP — opening this to get feedback on a refactor idea before taking it further.

prep_ngen_data in processor.py had grown to ~700 lines and was hard to follow. This is particularly a problem for anyone looking to work on/contribute to this code base.

This PR splits prep_ngen_data into an init step followed by named sequential steps, so the top-level function has much of the details abstracted out. This PR should not change functionality and is a pure refactor (besides maybe addressing a few bugs along the way).

Additions

  • config.py
  • writers.py
  • metadata.py
  • utils.py
  • Geometry / Extracted / WriteResult dataclasses to carry state between steps

Removals

  • All 16 module-level globals prep_ngen_data used to set
  • Duplicated forcings / channel_routing metadata dicts (identical keys, differed only in nvars_intput / nvars_output)

Changes

  • prep_ngen_data is now: read_configbuild_output_layoutparse_nwm_filenamesload_geometrywrite_run_manifestextractwrite_outputs → plot → collect_metadata → tar
  • data_source branching pushed down into named per-mode functions instead of ~20 inline if data_source == checks
  • processor.py: 1686 → 748 lines

Testing

All pytests pass besides unmerged fix for test_restart, pre-existing upstream off-by-one, unrelated to this PR)

Screenshots

Notes

  • Anything that hardcoded the old double-slash S3 metadata path will need updating.

Todos

  • Possible follow-up: split into three top-level entry points per data_source, since the troute_restarts path shares almost nothing with forcings

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal
  • Code can be automatically merged
  • Code follows project standards
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@JordanLaserGit JordanLaserGit added the P3 Priority level. P0: Critical, P1: High, P2: Medium, P3: Low label Aug 20, 2026
@quinnylee

Copy link
Copy Markdown
Collaborator

This is awesome and much more readable! Glad the global variables are gone. I really like the usage of the dataclasses to store our variables, maybe the config one can be used as an argument in the writers.py module to reduce the number of arguments passed around?

I tried to have three separate entry points for eachdata_source while I was hand-refactoring, but I found that there were just enough differences in the ways each function was called for each case that I wasn't able to neatly reuse functions. Or maybe I just didn't put enough time into it. Anyway, I don't think the three entry points are necessary for readability

@quinnylee quinnylee self-assigned this Aug 25, 2026
@JordanLaserGit

Copy link
Copy Markdown
Collaborator Author

I'm happy with any idea that reduces inputs args!

An idea I have for this in relation to the community contribution docs I'm working on, is it would be nifty to be able to link conceptually relevant portions of the code base in those docs.

For instance, the core processing function for forcings is forcing_grid2catchment() and this could be split into a few descriptively named functions with verbose comments describing the code. This effort would be entirely for visibility for contributors so they could readily look into the code and see what is scientifically relevant in this heap of a repository. Might be tricky just linking the line numbers, so maybe worth splitting into it's own file so we can reference reliably?

@quinnylee

quinnylee commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

We could also auto-generate API docs with links to source code if that would help with community contribution. Something like https://numpy.org/doc/stable/reference/generated/numpy.isdtype.html

So if we do break up big functions into little functions with good docstrings, those could accomplish the goal of exposing relevant sections through autogenerated API docs.

This could also be a chance for us to "privatize" our internal functions that may never change/get reused.

@JordanLaserGit

JordanLaserGit commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

@quinnylee I think that's a great idea. I can focus on breaking up forcing_grid2catchment() and others into scientifically relevant functions and probably stick them all in one file to be imported by the modules that need them.

I have the initial community contribution docs PR up here. I'm thinking the section "Alternative catchment averaging and weight generation" in CONTRIBUTING.md would contain the links we are talking about here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Refactors the main prep_ngen_data pipeline by splitting configuration, geometry loading, extraction, writing, plotting, metadata collection, and tar packaging into named steps and new helper modules, aiming to preserve existing behavior while making the flow easier to follow and maintain.

Changes:

  • Introduces config.py, writers.py, and metadata.py to encapsulate config parsing/output layout, output writing, and run-statistics generation.
  • Reworks processor.py into a step-based orchestration using new Geometry / Extracted / WriteResult dataclasses.
  • Stabilizes tests by restoring shared module-level configuration state between test cases via an autouse fixture.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
tests/test_forcingprocessor.py Adds autouse fixture to reset mutated shared conf between tests.
tests/test_channelrouting.py Adds autouse fixture to reset mutated shared conf between tests.
src/forcingprocessor/writers.py New module for dataframe/netcdf/tar writing and multiprocessing orchestration.
src/forcingprocessor/weights_hf2ds.py Import cleanup, typing tweaks, and small runtime hardening (timeouts, cpu_count guard).
src/forcingprocessor/utils.py Adds shared helpers (phase timing, JSON/dataset readers, work distribution/load balancing).
src/forcingprocessor/troute_restart_tools.py Adds typing ignore for boto3 client creation.
src/forcingprocessor/subset_conus_vpus.py Import cleanup and minor refactors/formatting.
src/forcingprocessor/processor.py Major refactor: step-based pipeline + new dataclasses + delegates to config/writers/metadata.
src/forcingprocessor/plot_forcings.py Typing improvements, safer defaults, and minor refactors to plotting helpers/CLI.
src/forcingprocessor/nwm_filenames_generator.py Improves file loading hygiene (encoding, variable naming).
src/forcingprocessor/metadata.py New module for metadata/statistics frame generation and output writing.
src/forcingprocessor/medium_range_time_ax_mod.py Docstring/header cleanup, constant naming, and typing adjustments.
src/forcingprocessor/config.py New module for config parsing/validation, output layout, and run manifest writing.
src/forcingprocessor/channel_routing_tools.py Minor cleanup and typing ignore for boto3 client creation.
Suppressed comments (1)

src/forcingprocessor/processor.py:613

  • Spelling: the status message says "processs" (extra 's'), which looks unintentional and makes logs harder to scan.
        print(
            f"\n\nWrite processs: {cfg.nprocs}\nWrite time: {write_time:.2f}" +
            f"\nWrite rate {geom.ncatchments / write_time:.2f} files/second\n",

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/forcingprocessor/processor.py Outdated
Comment thread src/forcingprocessor/processor.py Outdated
Comment thread src/forcingprocessor/processor.py Outdated
Comment thread src/forcingprocessor/writers.py Outdated
Comment thread src/forcingprocessor/writers.py
Comment thread src/forcingprocessor/weights_hf2ds.py Outdated
Comment thread src/forcingprocessor/writers.py Outdated
@quinnylee

Copy link
Copy Markdown
Collaborator

Sorry this has ballooned into an absolute monster of a branch. Let me know what you all think of the changes. Will be more than happy to revert (or you can go ahead and do it yourself since i'll be at conferences for like two weeks straight) if you think any of them are not useful!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Priority level. P0: Critical, P1: High, P2: Medium, P3: Low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants