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

Feature/graph analysis ipyleaflet visualisation #47

Merged

Conversation

rdnfn
Copy link
Member

@rdnfn rdnfn commented Mar 15, 2021

Introducing GeoGraphViewer 🎉 ... this PR adds the GeoGraphViewer visualisation class and a bunch of code that enables it. As the GeoGraphViewer is a fairly complex widget, this PR is somewhat lengthy, apologies in advance! Thanks a lot for your review!

Main changes:

  • All GeoGraph visualisation code now lives in visualisation/*. The old models/visualisation.py has been removed.
  • Added GeoGraphViewer class (in visualisation/geoviewer.py): this allows us to visualise GeoGraphs interactively.
  • Added control widgets (in visualisation/control_widgets.py): these make up the side control panel seen in demos. See architecture image below for a rough outline how they relate and are added to the GeoGraphViewer.

Other changes:

  • Added different utils that are used by GeoGraphViewer. These are in
    • visualisation/widget_utils.py
    • visualisation/folium_utils.py (old functions from visualisation.py, no need to review)
    • visualisation/graph_utils.py (old functions from visualisation.py, no need to review)
  • Added a new test case to data_loading/test_data.py
  • Added visualisation/style.py that contains constants defining the plotting style of graph, etc. in GeoGraphViewer.
  • Added ipyleaflet and jupyter lab dependencies in requirements/dev-requirements.txt
  • Added misc exploratory notebooks that helped me develop the viewer, for reviewers only notebooks/exploratory/rdnfn-8-polesia-geographviewer-demo.ipynb will likely be interesting, which is the demo on the Polesia data. Feel free to use it, it should work with the environment created via make env (if you updated it to new requirements)
  • The old folium code lives on without further development in the FoliumGeoGraphViewer class (in geoviewer.py)
  • Updated readme to new visualisation folder

Architecture
Screenshot 2021-03-15 at 20 51 38

GeoGraphViewer demo
Screenshot 2021-03-15 at 20 47 42

Add habitat layer selection to widget in GeoGraphViewer. Also includes other minor fixes for GeoGraphViewer
Added because of Jasmin server instability.
Also includes minor fixes:
- fix node color
- fix node, edge drawing order
…into feature/graph-analysis-ipyleaflet-visualisation
This factors the GeoGraphViewer control functionality into separate
widgets for better readability.
These traits can be used by multiple control widgets to determine their content.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Croydon-Brixton
Copy link
Collaborator

Love it already before reviewing! 😉 😍

node_gdf = gpd.GeoDataFrame(columns=["id", "geometry"])
rep_points = graph.nodes(data="rep_point")
for idx, rep_point in rep_points:
node_gdf.loc[idx] = [idx, rep_point]
Copy link
Member

Choose a reason for hiding this comment

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

Since .loc is quite slow, you should be able to speed this up quite a lot by just initialising the node_gdf using rep_points: https://www.geeksforgeeks.org/creating-a-pandas-dataframe-using-list-of-tuples/

Copy link
Member Author

@rdnfn rdnfn Mar 17, 2021

Choose a reason for hiding this comment

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

Good point! I added a performance improvement issue, so I don't forget it in the future. I think the cost of this compared to components is minor, therefore I will focus on other things first.

point_b = rep_points[node_b]
line = shapely.geometry.LineString([point_a, point_b])

edge_gdf.loc[idx] = [idx, line]
Copy link
Member

Choose a reason for hiding this comment

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

And similarly here, but you'll need to build the list of tuples in the loop first then make the edge_gdf from it at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

Copy link
Member

@herbiebradley herbiebradley left a comment

Choose a reason for hiding this comment

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

All done, looks good to me! I made some suggestions, a few are minor nitpicks and a couple more are performance improvements. Since time is tight there's no need to resolve those before the deadline if you're too busy.

@rdnfn
Copy link
Member Author

rdnfn commented Mar 17, 2021

Thanks a lot for the review @herbiebradley! Was able to address most points in 42eae9e, and created Issue #50 for the remaining performance improvements, to be dealt with at a later point.

@rdnfn rdnfn merged commit 2054afe into feature/graph-analysis Mar 17, 2021
GeoGraph automation moved this from In progress to Done Mar 17, 2021
@rdnfn rdnfn deleted the feature/graph-analysis-ipyleaflet-visualisation branch March 17, 2021 22:55
@rdnfn rdnfn restored the feature/graph-analysis-ipyleaflet-visualisation branch March 17, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GeoGraph
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants