uscuni / sgeop

Street geometry processing toolkit
BSD 3-Clause "New" or "Revised" License
13 stars 0 forks source link

end `simplify_network()` immediately if already meets criteria #68

Open jGaboardi opened 1 month ago

jGaboardi commented 1 month ago

We need some logic to end sgeop.simplify_network() immediately if the input roads dataset already meets our simplififed criteria. This can be determined within the call to sgeop.nodes.fix_topology() and simply return the result there.

The check should be after induce_nodes() is called where we are checking if roads and roads_w_nodes are equivalent. If they are, no further simplification is needed, as far as I understand.

Thoughts @martinfleis @anastassiavybornova ?

martinfleis commented 1 month ago

If they are, no further simplification is needed, as far as I understand

wrong. Topological correctness does not mean that there are no artifacts.

There is nothing to do if the two data frames are equal and the artifacts dataframe is empty.

jGaboardi commented 1 month ago

So that would be here after the first loop is completed, correct? (if that artifacts df is empty)

jGaboardi commented 2 weeks ago

When this condition is met, should it raise an error, or can we actually the input simplified enough and return? It seems like this could be true on both the first and second loops?

martinfleis commented 1 week ago

or can we actually the input simplified enough and return

We can't. Threshold detection occasionally fails in certain contexts due to the distribution of block shapes. It should raise as it does now.

jGaboardi commented 1 week ago

OK, I'm wondering if it would be prudent to cut a patch release of momepy after https://github.com/pysal/momepy/pull/666 so at least we can fail gracefully when an skeleton is passed in that can't be polygonized?

jGaboardi commented 1 week ago

After that I can look back to try to determine better logic this.

martinfleis commented 1 week ago

We'd need to backport that. But if you wait a bit, I think that Streetscape will be ready for 0.9.

jGaboardi commented 1 week ago

Yeah, I think this can surely wait. Just wanted to keep it on the radar.