Adding @karosc for some responses too. @aerispaha can you drop the link in for the draft built docs?

Also going to add @wraseman here for some thoughts too. He's a fresh SWMMIO user and it would be good to get his reaction.

@bemcdonnell and team, you can see the draft built documentation on readthedocs, here).

I see that there is a weird thing going on in the Making art with SWMM guide. I think it's because I have some caching going on. I'll fix that. Fixed.

@aerispaha, for starters, you've done such a nice job on the documentation overhaul! I like the focus and the new feel - lighter and cleaner. The only suggestions I have are for the User Guide. I think it would be useful to make a sub header section (BOLD) how to "Edit model Parameters." Get the attention of those who are quickly scrolling through that you can use this to Edit/modify models. Also, for running models, you might cite PySWMM as the vehicle that is used. Lastly, maybe consider a separate page for notebooks and add it to the TOC.

We can also provide documentation links from www.pyswmm.org if you'd like!

Keep up the great work on this! I think it is an asset to the community and so are YOU!

Working through the docs, providing feedback on README.md and the first two sections of Getting Started.

README.md

• Image: it isn't clear to me what the figure is supposed to be: docs/_static/img/swmm-zoom-graphic.png. Is it a logo? Is it a swmmio output? Would recommend one of the images in the docs (e.g., Visualizing SWMM Models — swmmio 0.7.4.dev0 documentation)
• Version number: there are three different version numbers README.md, swmmio.readthedocs.io, and releases. Which is correct?

Getting Started

• Dependencies: need to add requests or from swmmio.tests.data import fails

Getting Started: Edit Model Parameters

• The last DataFrame is intended to show how the user can modify the model and write it to a new .inp file. However, none of the modified values (i.e., OutfallType, StageOrTimeseries) are visible in the DataFrame shown in the tutorial.

Getting Started: Building Variations of Model

• For climate change scenario, I would have expected the stage to increase from 576 to 581 (5ft rise). But instead, it increases to 628.5 ft.
• In the same example, the 'baseline' variable gets overwritten in the loop. Since it is the baseline, shouldn't it remain unchanged? I'd recommend setting another variable, like 'slr'.

Picking up from where I left off on the Getting Started section.

Potential revisions to section navigation:

• Getting Started: no change
• Working with PySWMM: change to "Analyzing Timeseries Data from SWMM". Although PySWMM is used in this section, working with timeseries data from SWMM models seemed to be a stronger theme in this section.
• Visualizing SWMM Models: change to "Geospatial Visualization of SWMM Networks". There are visualizations throughout the User Guide, including NetworkX, GeoPandas, swmmio, and matplotlib. I found that this section was focused on spatial mapping.
• Making Art with EPA SWMM: trim down or potentially remove. I found this section fun, but I was surprised by how large it was compared to the other sections, especially because it didn't focus on core features of swmmio.

Getting Started > Nodes and Links

• Should 'geopandas' be added as a dependency for swmmio?

Working with PySWMM

• Potential name change (see above)
• Could use a couple sentences clarifying the difference in scope between PySWMM and swmmio
• Modify code for clarity?

Original

# Run Simulation PySWMM
with pyswmm.Simulation(model.inp.path) as sim:
  
  for i, step in enumerate(sim):
      
    # store each link's flow in a dictionary 
    link_flows[sim.current_time] = {
        link_id: pyswmm.Links(sim)[link_id].flow 
        for link_id in model.inp.conduits.index
    }

Proposed

# Run simulation PySWMM
def get_flow(link_id):
  return pyswmm.Links(sim)[link_id].flow
  
with pyswmm.Simulation(model.inp.path) as sim:
	  
  link_ids = model.inp.conduits.index
  
  for step in sim:
    # store each link's flow in a dictionary 
    link_flows[sim.current_time] = {
	      link_id: get_flow(link_id) for link_id in link_ids
  }

Visualizing SWMM Models

• Proposed name change (see above)
• More subsections to improve navigation
  • Network Visualization with swmmio
  • Network Visualization with geopandas
  • Example 1: Finding Important Outfalls
  • Example 2: Highlighting a Sub-Watershed
• Add axis labels to the geopandas plots for coordinates and legend for invert elevation.

Proposed

# draw the model links and outfalls
ax = model.links.geodataframe.plot(linewidth=model.links.dataframe['Geom1'], figsize=(10,10))
ax.set_xlabel('X Coordinate')
ax.set_ylabel('Y Coordinate')
outfall_plot = model.nodes.geodataframe.loc[outfalls.index].plot('InvertElev', ax=ax, legend=True, 
                                                                 legend_kwds={"label": "Invert Elevation", "orientation": "horizontal"})

• In 'Finding Important Outfalls' suggest changing # grab the list of outfalls to # grab dataframe of outfalls
• For dataframe, perhaps rename 'anscestors' to 'CountUpstreamNodes' for clarity. I find that the NetworkX terminology (i.e., edge, ancestor) is not intuitive in the context of analyzing SWMM models.

Proposed

# get a count of nodes upstream of each outfall
outfalls['CountUpstreamNodes'] = outfalls.apply(lambda x: len(nx.ancestors(G, x.name)), axis=1)
outfalls.sort_values(by='CountUpstreamNodes', ascending=False).head(n=10)

The Art section should be longer, IMO - After a hard day of H&H modeling, some of us may need to express ourselve with the other side of our brain.

_Why is it "Cool"?

• Novelty: It's a fresh and unconventional approach to both data visualization and art.
• Interdisciplinarity: It bridges the gap between engineering, science, and art.
• Visual Appeal: The dynamic nature of SWMM data can lead to visually stunning and captivating results.
• Raises Awareness: It can make people think differently about urban water systems and their importance.
• Creative Challenge: It's a fun and engaging way to push the boundaries of both your technical skills and your artistic imagination.

In essence, using swmmio for artistic purposes allows you to explore the beauty hidden within complex data, transform scientific simulations into aesthetic experiences, and create art that is both meaningful and visually compelling. It's about finding the art in the engineering and the engineering in the art._

@wraseman @dickinsonre @bemcdonnell, thanks so much for all this great feedback! I'm thrilled to have your input :)
I'll work on address these comments over the next couple days.

@wraseman, again, thanks for the thorough review. Below I will address your comments:

README image not clear what the it is supposed to be

I agree. this came from the making art with swmmio section and it's just something I liked aesthetically, but it doesn't make a ton of sense to be the opening image for swmmio. I've changed this to be a figure that shows flood levels in a model, generated with swmmio.

Version number: there are three different version numbers README.md, swmmio.readthedocs.io, and releases. Which is correct?

The development version is currently 0.7.4.dev0. We generally keep the version in the readme in sync with the latest released version of swmmio on pypi (currently 0.7.3). When we merge these changes and make a new release, I will update all of these version numbers to be in sync.

Dependencies: need to add requests or from swmmio.tests.data import fails

Done. thanks for catching this.

Getting Started: Edit Model Parameters

The last DataFrame is intended to show how the user can modify the model and write it to a new .inp file. However, none of the modified values (i.e., OutfallType, StageOrTimeseries) are visible in the DataFrame shown in the tutorial.

This is because of Pandas default display option truncating the visible columns. To address this, I've modified the example code to focus on the columns that have changed:

# see the changes in the higher-level nodes DataFrame
new_model.nodes.dataframe[['InvertElev','OutfallType', 'StageOrTimeseries']]

For climate change scenario, I would have expected the stage to increase from 576 to 581 (5ft rise). But instead, it increases to 628.5 ft.

Great catch. There was a little bug in the logic and it has been corrected. Now the last sea level rise scenario shows stages of 581.

In the same example, the 'baseline' variable gets overwritten in the loop. Since it is the baseline, shouldn't it remain unchanged? I'd recommend setting another variable, like 'slr'.

Good point. I think "baseline" in this context isn't is the best variable name. I've renamed the model that gets modified to simply model and name the final model version to a more descriptive model_slr_5, reflecting that that scenario includes 5 feet of sea level rise (SLR).

Second Batch of Comments

Working with PySWMM: change to "Analyzing Timeseries Data from SWMM".

I'm on the fence about this suggestion. I want to emphasize how these two tools can work well together, so I'd like to keep "PySWMM" in the title. I think this will make more sense as we add more use-cases for pyswmm/swmmio. I propose we add more such examples in a separate PR since this one has gotten pretty large.

Visualizing SWMM Models: change to "Geospatial Visualization of SWMM Networks".

Done, except I've slightly revised your suggestion to "Spatial Visualization of SWMM Models"

Making Art with EPA SWMM: remove or trim...

I'll address this in a separate comment.

Should 'geopandas' be added as a dependency

Geopandas is currently an "optional" dependency (in quotes because it isn't packaged as such properly). In a separate PR, we intend to overhaul/modernize the packaging of swmmio at which time I'd like to properly make geopandas an optional dependency.

Working with PySWMM

• Potential name change (see above)
• Could use a couple sentences clarifying the difference in scope between PySWMM and swmmio
• Modify code for clarity?

I've modified the code as you've suggested, with a couple small adjustments. And I've added a few sentences describing the scope of pyswmm/swmmio.

@wraseman regarding the Making Art with EPA SWMM: remove or trim suggestion.

I hear you that this section is not really necessary. But I do really appreciate @dickinsonre's feedback and point of view on this. In fact, these lovely words from @dickinsonre articulate my feelings on the matter probably better than I could myself:

In essence, using swmmio for artistic purposes allows you to explore the beauty hidden within complex data, transform scientific simulations into aesthetic experiences, and create art that is both meaningful and visually compelling. It's about finding the art in the engineering and the engineering in the art.

Yes, we probably should have more documentation on the primary use of swmmio (the hard engineering stuff). But, for whatever reason, I was inspired to explore some unconventional uses of swmmio a few weeks ago, and that's why that section is fairly long. In the future, maybe we can delineate a "Gallery" section or something, in which we can house some less conventional examples.

But for now, I'd like to keep the art section as is. Is that cool with everyone?

@aerispaha, all your responses look good to me! Thanks for thoughtfully considering my feedback. @dickinsonre, I appreciate your perspective on the art section--makes sense!