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

Update plot_network to use the geopandas plotting API in place of the networkx plotting API #451

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

kbonney
Copy link
Collaborator

@kbonney kbonney commented Oct 1, 2024

Summary

This PR updates the plot_network function by replacing networkx plotting functions with geopandas plotting functions. This builds on the geospatial capabilities of wntr by using the GeoDataFrames generated by to_gis() to interface with the geopandas plotting capabilities. This update reduces the complexity of the plotting code, making it easier to maintain and opening opportunities for other plotting options.

These updates aim to minimize changes to the output of plot_network, except where changes add functionality.
The arguments of plot_network remain unchanged, with the exception of some additional keyword arguments with defaults, so these updates should be fully backwards compatible.

New functionality includes:

  • Vertices of links are incorporated in the plot, rather than always having straight lines between nodes.
  • Valves, pumps, tanks and reservoirs are marked with distinct symbols. Direction of pumps is indicated by an arrow.
  • link_attribute and node_attribute string options are expanded to include attributes for all assets (pipe, valve, and pump for link and junction, tank, and reservoir for node)

Note that relying on geopandas for plotting will require elevating geopandas from an optional dependency to a required one.

This PR addresses #223 since geopandas plots the vertices, which means #390 is no longer necessary.

Tests and documentation

Current tests and documentation already cover the plot_network function.
An additional test in test_graphics is added to cover additional test cases. For development purposes, the old plot_network is included in this test file under the name plot_network_nx. In test_plot_network_options there is a variable compare which can be manually set to True to produce plots that side-by-side compare the old and new plotting functions. However, since we don't want to continue support the networkx plotting this isn't ran as part of the default test suite.

Acknowledgement

By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.

@coveralls
Copy link

coveralls commented Oct 1, 2024

Coverage Status

coverage: 84.417% (+0.05%) from 84.364%
when pulling 046006a on kbonney:gis_plotting
into c790b61 on USEPA:main.

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 5, 2024

Pending items to be resolved here or saved for later updates:

  • Customize asset markers (valve, pump, reservoir, tank) to match the shapes seen in EPANET. Could borrow code from viswaternet.
  • Current approach to showing pump direction as well as pipe direction when directed=True is sensitive to aspect ratio. It looks correct when the aspect ratio is 1, but if you stretch the image in the pop-up viewer the angles of the arrow directions are not updated, so they look off. Another approach could be taken using matplotlib arrow objects to avoid this.
  • BUG: An issue arises in the earthquake demo due to an unexpected interaction when using to_gis() on a WaterNetworkModel with leaks. This isn't caused by the code introduced by this PR as far as I can see, rather this interaction wasn't covered by the test suite previously, but now is caused by the use of to_gis in plot_network. See Exception occurs when running to_gis() on a WN with leaks #456 for details.

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 5, 2024

Hi @meghnathomas, it seems I cannot directly add you as a reviewer to the PR. This probably has to do with some repository level restriction on who can be added as a reviewer. I'll see what I can do to get you added, but let me know if it causes any problems with the review in the meantime.

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 13, 2024

Failing tests should be addressed when #458 is pulled in.

@kbonney kbonney marked this pull request as ready for review November 13, 2024 23:40
@angusmcb
Copy link
Contributor

Will this implementation keep geopandas as an optional dependency, only needed if plotting ? Would be much appreciated if this is the case :-)

@meghnathomas
Copy link

Hi, sorry for the delay on the review! Here's some thoughts/comments:

  1. For me, often (but not always!) the color bars seem to be normalized to 1 when I use the new geopandas code. E.g., these are the plots I get when I run test_plot_network2 in test_graphics.py (plotting directed and undirected plots of node elevation and link length)
geopandas plotting undirected networkx plotting undirected
image image
geopandas plotting directed networkx plotting directed
image image

Similarly, these are the results from test_plot_network4 :

geopandas networkx
image image

But when we run test_plot_network5, the plots match. The major difference seems to be that we specify "node_range" when we call the function here.

geopandas networkx
image image

Am I the only one seeing these discrepancies? These are the package version I'm working with on my machine:
matplotlib 3.7.1
geopandas 0.14.4
numpy 1.26.4
pandas 2.2.3
networkx 3.3

  1. It might be useful to add a legend (that you can choose to show or hide) for the pumps (triangles? arrows?), valves (a line?), tanks (diamonds?), and reservoirs (squares?) so they can be differentiated easily!

  2. The directed graphs are a lot more visually pleasing in the geopandas version. They might be a little easier to visualize if the arrows were slightly smaller, or could somehow be scaled to the fig size. Not a huge deal at all, just noting for future possible enhancements. (looks like you mentioned this in your first comment here on this thread)

  3. In the third set of plots above (when we use network Net1), in the geopandas plot the color bars seem to squeeze the network layout a little bit but the networkx version manages to fit the color bars and plots together quite nicely

  4. It also looks like for the networkx plots, wntr automatically labels the color bars "Node" or "Link" (which is a nice touch) but that doesn't seem to happen for the geopandas plots

  5. It's very cool that you guys plot vertices too now. Looks great!
    image

  6. Is there any way to disable valve/pump/reservoir/tank plotting? Like for this network, BWFLNet, there are about 500 throttle control valves in the network and the graphic is a little hard to look at:
    image

  7. Do you also plan to show valve direction (like you show pump direction)?

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 20, 2024

Hi @meghnathomas, thanks for your rigorous testing of the PR. I'll work through the items you pointed out and get back to you with some updates .

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 20, 2024

@meghnathomas,

  1. Good catch, this should be fixed by the latest commits.
  2. I agree this would be helpful. I will have to spend more time looking into this.
  3. I agree that the arrows were too large. I changed them so they are more like the size in the networkx version.
  4. The cbars should have the correct spacing now.
  5. This originally was an intentional choice by the team. I think it looks okay when only plotting one colorbar, but it is confusing if you have both. I'll revisit this with the others.
  6. Yes! This PR originated as an effort to add vertices in the networkx framework. It became a pain so it started to make more sense to use the geopandas plotting capabilities, which are much more flexible and user-friendly.
  7. I added additional keyword args for this. They default to not show pumps and valves so that previous plotting code doesn't retroactively add them.
  8. This should be simple to add. I will look into it.

I also caught a bug where labels only rendered for junctions/pipes rather than all assets.

@meghnathomas
Copy link

@kbonney awesome, thanks, everything looks good with the colorbar values, spacing b/w plot and colorbar, arrow sizes, and suppressing pump/valve plotting!

A couple of comments:

  1. I think in the networkx version, when alpha < 1, the color bar alpha would match the marker colors alpha, but i think in this geopandas version alpha only applies to the markers and not to the color bar
  2. Are the tanks, reservoirs, pumps, and valves supposed to show up by default on all plots, even when you specify node_attribute/link_attribute? Or are they only supposed to show up on a basic network layout plot with no data assigned to nodes/links? If a user specifies "node_attribute" and the list of nodes they are plotting values for doesn't contain the tanks and reservoirs, those nodes don't show up on the map so I don't know where the tanks/reservoirs are located. Is that an intentional choice? If so, that's fine, just wanted to check! Noticed this when I used the following kwargs in your new test_graphics file:
{"node_attribute": node_list},
{"node_attribute": random_node_values},
{"node_attribute": random_node_dict_subset}

I didn't look closely enough at the figures to check if this is true for pumps (and valves) too, but this question applies to them as well

@kbonney
Copy link
Collaborator Author

kbonney commented Nov 26, 2024

Thanks for the additional comments @meghnathomas:

  1. Fixed this, the alpha should now carry over to the cbars.
  2. All nodes are always plotted, unless a subset of nodes is specified in which case only that subset is plotted. This includes junctions, tanks, and reservoirs. The only difference is that tanks/reservoirs have a different shape and size.
    This should also be the case for links (pipes, pumps, valves). The edge of a pump or valve is always plotted by default, but the additional visual indicators are only shown when plot_pumps or plot_valves is set to True.

For both nodes and links, all assets should be shown unless a list or Series is passed in as the attribute. In that case, only the asset names that show up in the list or Series index will be shown.

@kbonney
Copy link
Collaborator Author

kbonney commented Dec 3, 2024

Summary of recent updates:

  • Removed pump/valve direction code. This feature could be added back in a later update, but for now I want to prioritize getting a working version of the new function that is backwards compatible.
  • When a string is provided as an attribute it is used as the default label.
  • Added a legend. The legend chooses some color from the plotted elements as a representative for the symbol. Ideally we would set the color to always be black for consistently, but overriding this behavior has proven difficult.
  • All nodes are now plotted regardless of attribute. If a node is not included by the provided attribute the color defaults to black. Similar behavior for links.
  • Aspect set to "equal" instead of the default "auto". This forces plots to maintain consistent x/y axis ratio regardless of zoom or the shape of the plotting window.

@meghnathomas
Copy link

meghnathomas commented Dec 6, 2024

@kbonney this looks awesome and works super well! :-) Sorry to continue to pester you with comments, but I have just a couple more based on the most recent version of the code:

  1. It looks like the colorbar titles can sometimes overlap a little - is that because the aspect ratio is now "auto" you think?
    image
  2. Thank you for adding the legend! Please feel free to disagree with this comment or leave this task for another time - but it might also be useful to allow users to customize legend location so they can move it around the plot if they want. Providing a default nice legend location that's always out of the way of the network plot no matter what the shape of the network is is also an option but a lot more work on your end probably (that's why we didn't do that for viswaternet haha). In any case this is not critical at all!

@kbonney
Copy link
Collaborator Author

kbonney commented Dec 9, 2024

Will this implementation keep geopandas as an optional dependency, only needed if plotting ? Would be much appreciated if this is the case :-)

@angusmcb, geopandas would become a required dependency as plotting is considered a core WNTR functionality. Geopandas has had some installation difficulties in the past, but many of those seem to have been taken care of. What is the reason you wouldn't want to install geopandas for WNTR?

@angusmcb
Copy link
Contributor

I've been working on a plugin for QGIS (currently experimental) which effectively replaces the wntr gis module and geopandas with PyQGIS:
https://github.com/angusmcb/wntr-qgis/

I did think that Geopandas wasn't included as a dependency as standard with QGIS, but have now realised that it is included with the most recent QGIS (windows) releases, so no problem!

@kbonney
Copy link
Collaborator Author

kbonney commented Dec 12, 2024

I've been working on a plugin for QGIS (currently experimental) which effectively replaces the wntr gis module and geopandas with PyQGIS: https://github.com/angusmcb/wntr-qgis/

I did think that Geopandas wasn't included as a dependency as standard with QGIS, but have now realised that it is included with the most recent QGIS (windows) releases, so no problem!

Thanks for the clarification. It is exciting to hear about the QGIS plugin - looking forward to its progress!

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