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

Liège issues review #138

Open
anastassiavybornova opened this issue Aug 4, 2024 · 8 comments
Open

Liège issues review #138

anastassiavybornova opened this issue Aug 4, 2024 · 8 comments
Assignees
Labels
algo/method bug Something isn't working enhancement New feature or request

Comments

@anastassiavybornova
Copy link
Collaborator

starting to collect review of remaining simplification issues for Liège (branch check-issues-00)

@anastassiavybornova anastassiavybornova added bug Something isn't working enhancement New feature or request algo/method labels Aug 4, 2024
@anastassiavybornova anastassiavybornova self-assigned this Aug 4, 2024
@anastassiavybornova
Copy link
Collaborator Author

anastassiavybornova commented Aug 4, 2024

singles

image

image

Feature request: can we (at a later stage) "smoothen out" such new lines? xref uscuni/sgeop#16 --> if we preserve attributes and know which are the "new" edges, we can check those for their smoothness


image

some dangling happened here (--> will we have a post processing step of dropping these short dangling edges? again, will be good to have the "are they new or not" binary) -> to be solved by #140


image

just checking: non-planarity >> nothing happened >> all good >> right? :)


@anastassiavybornova
Copy link
Collaborator Author

anastassiavybornova commented Aug 4, 2024

doubles


image

is ok - but would be even better if treated as cluster? (this is an elongation issue maybe)


image

non-planarity >> ok i think?


image

image

closed loop introduced in the above cases >> not what we want? should we have a check for this?


image

image

need to check whether this is non-planarity or... why the artifact is not removed

@anastassiavybornova
Copy link
Collaborator Author

clusters


cluster comp 5:

image

this line (highlighted in pink) shouldn't be there. no clue why this happens.

image

would be great if those "new" artifacts would also be detected in a 2nd step! we can try with some version of FaceArtifacts.

@martinfleis
Copy link
Contributor

martinfleis commented Aug 5, 2024

Feature request: can we (at a later stage) "smoothen out" such new lines?

We partially do that by calling simplify but we can see if there's something to do here. Maybe visibility-based COINS may help? Or maybe bumping the simplification factor might be enough. but we need to be careful a bit to ensure it stays within the geometry in all cases.

some dangling happened here (--> will we have a post processing step of dropping these short dangling edges?

No, the processing of dangles is already included in nx_gx. I'll have a look at this one.

just checking: non-planarity >> nothing happened >> all good >> right? :)

Yes

is ok - but would be even better if treated as cluster? (this is an elongation issue maybe)

I don't think so. What happens here is that we enforce a connection to the highest degree node, which happens to be pretty far from the incoming nodes. I would probably include some heuristics to prefer the closer node if there's so big difference as is here.

non-planarity >> ok i think?

I would need to see which nodes are actually there. It may or may not be fine. If the only non-planar is the light blue, the stuff "under" should be simplified.

closed loop introduced in the above cases >> not what we want? should we have a check for this?

I am aware and wasn't able to prevent this. The idea is that a second run will eliminate them.

need to check whether this is non-planarity or... why the artifact is not removed

That is the similar case as above. The stuff "under" might need to be simplified but because it is marked as non-planar, it is skipped

this line (highlighted in pink) shouldn't be there. no clue why this happens.

Will have a look

would be great if those "new" artifacts would also be detected in a 2nd step! we can try with some version of FaceArtifacts.

I would simply do a second run with the same artifact threshold used for the first one. but the pipeline does not include that yet.

Not sure if two loops will be enough, now thinking about it.

@martinfleis
Copy link
Contributor

is ok - but would be even better if treated as cluster? (this is an elongation issue maybe)

I don't think so. What happens here is that we enforce a connection to the highest degree node, which happens to be pretty far from the incoming nodes. I would probably include some heuristics to prefer the closer node if there's so big difference as is here.

Actually, this is wrong since there's another entry point on the other (top) part. Maybe cluster solution might be the best even though it does alter the geometry of C here. Not sure, up to a discussion.

@martinfleis
Copy link
Contributor

need to check whether this is non-planarity or... why the artifact is not removed

We'll need to fix this. Now we exclude these from solving because they are non-planar but we should just ignore that one line crossing the artifact and simplify the geometries underneath.

@anastassiavybornova
Copy link
Collaborator Author

anastassiavybornova commented Aug 7, 2024

Current state of above issues:

Singles 365, 607

  • should connect to C rather than to the node on C -- do a tri-fork and pick shortest of the 3 connections
  • still some wobbliness left -- increase the relevant parameter for this (can't recall which one, but iykyk)
  • merge nodes -- osmnx-like solution with dbscan on the combination of new and changed edges

Doubles comp 65

  • due to the large/high/long...? elongation the currently applied solution is not the optimal one
  • idea is to do an all to all weighted (by edge length) shortest path comparison on all target nodes of each simplified artifact (that is elongated "enough". threshold to be played around with) and highlight as error if shortest path length increases "drastically" (again tbd what that means numerically)

(xref Wuhan #141 doubles comp 39, caused by different strategy but suggested detection of the error would be the same)

Doubles comp 88

If the connection line between 2 artifacts that together form a double is also what causes both of them to be classified as non planar -- merge and treat as cluster (to preserve nonplanarity but still solve artifact)

Cluster comp 5

Rest of singles/doubles/clusters -- solved

@anastassiavybornova
Copy link
Collaborator Author

some of the above potentially deserves to be an issue in its own right. just sharing notes here. same for wuhan issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
algo/method bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants