Skip to content

Swmmio#407

Draft
barneydobson wants to merge 14 commits intomainfrom
swmmio
Draft

Swmmio#407
barneydobson wants to merge 14 commits intomainfrom
swmmio

Conversation

@barneydobson
Copy link
Copy Markdown
Collaborator

@barneydobson barneydobson commented Apr 25, 2025

Description

This PR proposes the general structure for switching from my hacky SWMM file writing functions to swmmio (a devoted package for SWMM file IO). This also facilitates easy customisation of SWMM model file writing. The iterate_io function, when complete, will replace synthetic_write.

I keep the SWMManywhere design philosophy of:

  • Use a registry and list item of what to iterate over.
  • Iteratively transform the object (a swmmio model object now not a graph), passing necessary parameters and filepaths.
  • Enable custom modules in the same way as other custom modules.

If this looks good - I will convert the rest of post_processing into this format and switch over to use iterate_io in the main swmmanywhere call. Once approved, subsequent PRs on the topic will be to merge into this branch.

Fixes: #84

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 25, 2025

Codecov Report

Attention: Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.27%. Comparing base (e39f8cb) to head (a6be8e9).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
src/swmmanywhere/post_processing.py 95.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   88.10%   88.27%   +0.16%     
==========================================
  Files          23       23              
  Lines        2287     2337      +50     
  Branches      288      294       +6     
==========================================
+ Hits         2015     2063      +48     
- Misses        193      194       +1     
- Partials       79       80       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@barneydobson barneydobson requested a review from Copilot April 25, 2025 15:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new I/O structure for SWMM file processing by replacing the old synthetic_write function with a more modular swmmio‐based approach and additional testing.

  • Implement new I/O functions registered through a decorator and managed by an io_registry.
  • Add a custom parameter group and update configuration schemas to support custom I/O modules.
  • Update tests to validate model construction and I/O registration.

Reviewed Changes

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

Show a summary per file
File Description
tests/test_post_processing.py Added new tests and fixtures to verify the updated post processing using swmmio.
tests/test_data/custom_io.py Introduces a custom I/O function registered via the decorator.
src/swmmanywhere/swmmanywhere.py Adds a new function for registering custom I/O modules and integrates it into config loading.
src/swmmanywhere/post_processing.py Implements I/O registry, new I/O functions (e.g., apply_nodes, iterate_io) and a helper method.
src/swmmanywhere/parameters.py Adds a new parameter group for post processing.
src/swmmanywhere/defs/schema.yml Updates the schema to include custom_io_modules.
pyproject.toml Adds the swmmio dependency.
Comments suppressed due to low confidence (1)

src/swmmanywhere/post_processing.py:79

  • Reference to 'gpd' is used without being imported. Please add 'import geopandas as gpd' at the top of the file.
nodes = gpd.read_file(addresses.model_paths.nodes)

@cheginit
Copy link
Copy Markdown
Collaborator

The main issue is that swmmio is not available on conda-forge, so if you add this as a dep, we cannot release a new version on conda-forge anymore. We either have to ask the developers of the package to add it to conda-forge, or find an alternative.

@barneydobson
Copy link
Copy Markdown
Collaborator Author

barneydobson commented Apr 28, 2025

Ah OK thanks for raising that. For now it will be to mimic the old behaviour - so perhaps there can be optional imports/behaviour?

I can also just delay this - it's not urgent. It's for a new case study I'm working on, but I can equally keep that code separate

@dalonsoa
Copy link
Copy Markdown
Collaborator

I guess it is not possible to have a conda-forge package with dependencies living in pypi, right?

@cheginit
Copy link
Copy Markdown
Collaborator

cheginit commented Apr 28, 2025

I just opened an issue on their repo here.

@dalonsoa Right, all dependencies (including build deps like compilers and libraries) must be available on conda-forge.

@cheginit
Copy link
Copy Markdown
Collaborator

cheginit commented May 5, 2025

They don’t seem to plan on adding it to conda-forge, so we need to weigh whether including swmmio as a dependency is worth dropping conda-forge support. With 2K installs from conda-forge so far, the user base there is non-trivial.

EDIT: It seems that we're back on track with swmmio!

@dalonsoa
Copy link
Copy Markdown
Collaborator

dalonsoa commented May 6, 2025

Yep, for what it seems in the issue you open, the look quite enthusiastic and open to do so. That's good news.

Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

This looks good, but I have a couple of comments. More important, I'm not entirely sure what's the advantage of this over what you had before. Could you elaborate? It might be part of the documentation, as well.

nodes["id"] = nodes["id"].astype(str)
nodes["max_depth"] = nodes.surface_elevation - nodes.chamber_floor_elevation
nodes["surcharge_depth"] = 0
nodes["flooded_area"] = 100 # TODO arbitrary... not sure how to calc this
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe add an issue about it, if you haven't done so, yet.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I have just moved these to the 'default SWMM parameters' yaml instead, as they make more sense there for now. I guess ultimately everything will be moved to be a SWMManywhere parameter in parameters.py, but that will be in a different PR

@barneydobson barneydobson mentioned this pull request May 16, 2025
10 tasks
@barneydobson barneydobson marked this pull request as draft May 21, 2025 14:47
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.

Convert inp file read/write to swmmio

5 participants