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

end simplify_network() immediately if already meets criteria #68

Open
jGaboardi opened this issue Oct 27, 2024 · 9 comments
Open

end simplify_network() immediately if already meets criteria #68

jGaboardi opened this issue Oct 27, 2024 · 9 comments

Comments

@jGaboardi
Copy link
Collaborator

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
Copy link
Contributor

martinfleis commented Oct 28, 2024

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
Copy link
Collaborator Author

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

@jGaboardi
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@jGaboardi
Copy link
Collaborator Author

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

@martinfleis
Copy link
Contributor

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

@jGaboardi
Copy link
Collaborator Author

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

@jGaboardi
Copy link
Collaborator Author

So I have been playing around with some ideas and thinking on this, and (unless I am missing something obvious) the logic gets a bit subjective. We may simply want to make it clear to the user that the algorithm we employ does not decide if a network dataset is "simplified enough" and the user themself needs to determine that.

As a case in point, below is some code and plots of the original Apalachicola, FL testing data (SI-0) and three successive rounds of simplification (SI-{1,2,3}). SI-{1,2} are both clean results, with SI-2 being slightly over-simplified. However, SI-3 leads to dramatic over-simplification while not being "wrong." It should be noted that a fourth round of simplification fails with our current logic

My goal with this was to find some simple & clear criterion for popping out of the process, but this is seems to be more difficult than I had imagined These are some relevant changes on the @GH68_early_terminate_if_all_good branch in:

import geopandas, matplotlib.pyplot, sgeop

f = "sgeop/tests/data/apalachicola_original.parquet"
raw_original = geopandas.read_parquet(f, columns=["geometry"]).reset_index(drop=True)

iterations = 3
scene = "Simplification Iteration {}"
scenes = {scene.format(0): raw_original}

for i in range(iterations):
    scenes[scene.format(i+1)] = sgeop.simplify_network(scenes[scene.format(i)].copy())

panels = [[scene.format(0), scene.format(1)], [scene.format(2), scene.format(3)]]
fig, axd = matplotlib.pyplot.subplot_mosaic(panels, figsize=(9, 9), layout="constrained")
for k, ax in axd.items():
    scenes[k].plot(ax=ax)
    ax.axis("off")
    ax.set_title(k)

Perhaps we should leave this issue open for now, but I'll not spend more time on it at this point.


simplification_iterations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants