Add snap_x/y_tolerance and join_x/y_tolerance table-extraction settings #553
Conversation
Based largely on @dustindall's work in PR #51, adapted to current code. Also resolves issue #475.
Now all tolerance settings have x/y versions as well. This commit also changes `table.merge_edges(...)` behavior when `join_tolerance` (and `x`/`y` variants) `<= 0`, so that joining is attempted regardless, to handle cases of overlapping lines.
Codecov Report
@@ Coverage Diff @@
## develop #553 +/- ##
========================================
Coverage 98.79% 98.80%
========================================
Files 10 10
Lines 1249 1253 +4
========================================
+ Hits 1234 1238 +4
Misses 15 15
Continue to review full report at Codecov.
|
| @@ -59,16 +65,18 @@ def get_group(edge): | |||
| else: | |||
| return ("v", edge["x0"]) | |||
|
|
|||
| if snap_tolerance > 0: | |||
| edges = snap_edges(edges, snap_tolerance) | |||
| if snap_x_tolerance > 0 or snap_y_tolerance > 0: | |||
There was a problem hiding this comment.
Shouldn't it be an and comparator? And should <=0 values be treated differently?
There was a problem hiding this comment.
I don't think so? I think you want to run this block as long as any positive snap tolerance is specified. Or perhaps I misunderstand the concern?
Re. <=0 values:
-
0should be treated, I think, as indicating "do not snap" and thus no additional code should need to be run. -
I suppose we should convert negative values to
0. I'll push some code that does that. Unless you see a strong purpose for a negative tolerance here?
There was a problem hiding this comment.
On negative values, decided instead just to raise an error if they're negative — which gives us more flexibility to change the behavior in the future (rather than silently modifying the user's initial parameters). See aa2d594 below.
| edge_groups = itertools.groupby(_sorted, key=get_group) | ||
| edge_gen = ( | ||
| join_edge_group(items, k[0], join_tolerance) for k, items in edge_groups | ||
| _sorted = sorted(edges, key=get_group) |
There was a problem hiding this comment.
Should the join_x_tolerance > 0 and join_y_tolerance > 0 check be added here as well?
There was a problem hiding this comment.
My apologies, I could/should have been clearer about this. This is what I meant by this part of the PR message:
This PR also changes
table.merge_edges(...)behavior whenjoin_tolerance(andx/yvariants)<= 0, so that joining is attempted regardless, to handle cases of overlapping lines (which would still fulfilltolerance == 0). This is an uncommon situation but is, I believe, represents the user-expected behavior.
Essentially, join_edge_group(...) is still helpful, even when join_tolerance==0, because it combines ("joins") overlapping line segments that lay along the same infinite line.
|
Merged, but I'm open to reverting/changing anything that seems off before releasing the next version. |
Based largely on @dustindall's work in PR #51, adapted to current code. Also resolves issue #475.
This PR also changes
table.merge_edges(...)behavior whenjoin_tolerance(andx/yvariants)<= 0, so that joining is attempted regardless, to handle cases of overlapping lines (which would still fulfilltolerance == 0). This is an uncommon situation but is, I believe, represents the user-expected behavior.