-
Notifications
You must be signed in to change notification settings - Fork 12
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
[WIP] Simplify Graph #58
base: master
Are you sure you want to change the base?
Conversation
This is a great idea, thanks for working on it. Concerning your TODOs:
I think it should return something like We could add a parent type to Ideally the NodeIDs should be the same as then we can use all the additional OSM data for them - are they?
Yep this is pretty important, but hopefully just a small extension to what you've got already.
Does my answer to 1 above remove all the instabilities, or do other functions need improving as well? |
src/simplification.jl
Outdated
weight = Vector{eltype(osmg.weights)}(), | ||
geom = IGeometry[], | ||
) | ||
node_gdf = DataFrame(id = Int[], geom = IGeometry[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in thinking that the use of IGeometry
, createpoint
and createlinestring
are not necessary for the graph reduction, they just give more meta data? If that's the case then I don't think they should be here. They could perhaps be added in new functions for all OSMGraph
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should return something like SimplifiedOSMGraph where the graph type and other parametric types match that of the input OSMGraph.
Yes, adding a dedicated type is probably necessary .
If that's the case then I don't think they should be here. They could perhaps be added in new functions for all OSMGraphs.
I would also prefer to keep the geometric types out, so one can decide on their own which library to use.
When the SimplifiedOSMGraph
keeps a reference to the parent, it would even be sufficient to just store a list of all the interstitial NodeIDs from the original graph for every new edge.
Generating the geometries could then be achieved by indexing OSMG.node_coordinates
with that list
IMHO that would also solve most issues related to TODO 2 and 3.
They could perhaps be added in new functions for all OSMGraphs
Agree, I already implemented two function generating something similar for plotting with Plots.jl . I'll upload them together with the example script.
I'll keep you updated about further ideas!
Hey sorry for the delay, will try to review soon. It'd also be good for @captchanjack to take a look at this as it's a relatively big addition and I'm keen to know how it fits in with his thoughts for the package. |
It would be helpful to fix the merge conflicts via a rebase or merge - in particular the renaming of the field |
I've merged the changes, and added the |
@rush42 Just wanted to say sorry we haven't got around to reviewing this yet. Hopefully someone will be able to soon! |
Awesome PR! will this be merged soon? I've tried using as is but cannot get |
@ctrebbau yes I haven't worked on the shortest path algorithms yet. And right now I can't find the time to do so. IMO it's the only thing missing, the rest should be implemented... |
This PR implements an algorithm for simplifying the topology of an
OSMGraph
object.It is adapted from osmnx.
This PR is work in progress, and a few issues have to be discussed.
TODO:
DiGraph
and twoDataFrames
: one for nodes and the other for edges (which also contains the edge geometry) )Below is an example for Tiergarten district in Mitte, Berlin, Germany:
Before the simplification:
nodes: 2976,
egdes: 4727
After the simplification:
nodes: 683,
edges: 1384
I will upload an example script the following days.