-
Notifications
You must be signed in to change notification settings - Fork 373
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
Use Ozaki et al.'s error bound and single-branch evaluation in orientation index filter. #1184
Conversation
Thanks for providing a PR for JTS well. It's correct that the two libraries sometimes diverge in implementation, but it's nice to keep them in synch as far as possible, to reduce the mental complexity of future debugging. |
…ation index filter.
57586ed
to
8e4e03e
Compare
@tinko92 it would be nice to have a stress test in GEOS (and JTS) for point-line orientation, to confirm that the implementation is working as expected. Do you know of anything like this? The tricky part of course is to generate test cases with known correct answers. This could be done either a priori, by careful construction of the cases, or via some kind of higher-precision (but presumably slower algorithm). |
@dr-jts The implementation here changes the filter. The filter's correctness criterion is to return either the correct sign or failure so an error case would be returning the wrong sign, returning failure would never be incorrect (though, potentially a performance regression, should never occur with this filter without also occuring with the old one). The entire predicate runs an exact stage iff failure is returned, so any error case of the filter is an error case of the entire predicate so testing the entire predicate is sufficient (and strictly better coverage). The degenerate CCW tests currently in the test suite do some of that. To stresstest the implementation I would run many degenerate cases (so maybe not as a unit test). Off the top of my head, I would suggest the following generators of degenerate cases:
Potential oracles to test the results against are:
Stresstest without an oracle: The generators can be extended to return four points instead of three. The predicate would then not be tested for correctness but for consistency, i.e. something roughly like for ( auto [p1, p2, p3, p4] : generator() ) for ( auto [pa, pb, pc, pd] : permutations(p1, p2, p3, p4) )
if ( (pa == pb || pa == pc || pb == pc) )
{
if (orientation(pa, pb, pc) != 0) throw ...;
else continue;
}
if ( orientation(pa, pb, pc) != -orientation(pa, pc, pb) ) throw ...;
if ( orientation(pa, pb, pc) == 0 )
{
auto [pa_, pb_, pc_] = sort_lexicographically(pa, pb, pc);
if ( orientation(pa_, pb_, pd) != orientation(pa_, pc_, pd) ) throw ...;
else continue;
}
auto [pa_, pb_, pc_] = sort_to_ccw(pa, pb, pc);
if( orientation(pa_, pb_, pd) == right
&& orientation(pb_, pc_, pd) == right
&& orientation(pc_, pa_, pd) == right ) throw ...;
} I hope, I did not miss anything there. Any correct predicate will be consistent. I would be very surprised if any incorrect predicate (except something trivial like always returning the opposite of the correct sign or always 0) could realistically be consistent. This has the advantage of not needing external code and consistency is the property one actually cares about. If you would like some or all of the above in geos (generators 1-4, test for correctness with external dependency or consistency without or both), I could write that as a PR later when I have time (maybe for the benchmark directory? Because I think to be useful this would be long-running and should not run by default through If test cases should be generated together with results from oracles and then stored in text, it would be very important to print the coordinates after std::cout << std::setprecision(std::numeric_limits::max_digits10); |
Thanks for the detailed reply. I agree that for the orientation predicate consistency is at least as important as correctness. Your suggestion of a consistency checker is very interesting. A PR would be welcome. On a related note, I had hoped that the relaxed filter bound would resolve this issue, where the DD orientation result is inconsistent with the FP result, and from a human perspective obviously incorrect:
But this still produces the visibly incorrect answer of I've been wondering about always running the orientation computation first in FP, and accepting that answer if it is 0 (point lies on the line). This should avoid inconsistencies such as the above. @tinko92 any thoughts on this? |
@dr-jts For the given example, the filter returns failure (uncertainty) and the exact computation returns non-collinearity, which is correct for the closest floating-point approximations of (rd(0.4) rd(1.6)), where rd(x) is the function that rounds to nearest double. Behaviour under different orientation filters and exact orientation predicateA point that is collinear (green) with (2 0, 0 2) and has the y-coordinate rd(1.6) exists but its x-coordinate is slightly smaller than rd(0.4). In the following, I visualised the area of filter failures (yellow) respectively for Ozaki's filter, an interval arithmetic filter (slower) and exact computation (much slower), with Point(rd(0.4), rd(1.6)) marked by a black dot. Filter as orientation predicate with toleranceYou could change the filter's return value for uncertainty cases from failure (trigger DD computation) to collinear (then DD computation would just never be called) to obtain a predicate with tolerance. This predicate will return collinear for many but not all inputs where decimal approximation would suggest collinearity. To obtain a tolerant filter that guarantees returning collinearity on inputs that are collinear in decimial, you also need to account for rounding of the input values (which Ozaki's filter does not do) and you obtain an error bound of the form ~6e-16 ( (abs(a.x) + abs(c.x)) * (abs(b.y) + abx(c.y)) + (abs(a.y) + abs(c.y)) * (abs(b.x) + abx(c.x)) ), iirc. This would be similar to Burnikel et al.'s filter, with a much larger tolerance region but still on the relative order of 10 times machine epsilon (canvas zoomed out compared to above ones). New problems/side effects expected from that approachThis approach would definitely not be consistent for more than three points, e.g. pc might be judged not-collinear to line(pa, pb) under tolerance but for some pd that is far away from pa, pb and pc and thereby induces a much larger tolerance area, both pb and pc could be judged collinear to line (pa, pd) under tolerance. The approach of rounding determinants smaller than some epsilon-dependent bound to zero is discussed by Kettner et al. as "epsilon-tweaking" in Section 6 Non-Solutions. Recommendation and WarningIf you want to use such an orientation test with tolerance I would exclusively use it for simple predicate algorithms (like point-in-linestring predicate, etc.) but never for construction algorithms that rely on consistency in non-trival ways (like triangulation, convex hull, polygon clipping etc.), due to the discussion by Kettner et al., and still expect some non-intuitive situations, e.g. a user computes an intersection point of line a and b, which has a larger accumulated error than you would get from just rounded decimal inputs of an actual intersection point, the predicate with tolerance judges the computed intersection point non-collinear with lines a and or b. The user then prints the computed intersection point to stdout with default precision (like 6 digits?) that masks the accumulated error from the intersection computation, tests the point directly against the line again and then the predicate suddenly returns collinearity. Potential rigorous (but difficult) alternative without above problemsI believe (I am not aware of any implementation), a geometric predicate that is 1) consistent, 2) returns intuitive results for decimal inputs, and 3) not orders of magnitude slower than what you have now could work like this:
Extending this to constructions (e.g. also computing intersection points as approximations and in decimal if needed for set operations) would be a major undertaking, though. |
@tinko92 once again, thanks for your detailed and informative comment. Agreed that any approximated orientation test should only be used for spatial predicates (where topology is only evaluated at points, so overall consistency is less (or non) critical. I'm still unclear whether it is reasonable to use an approach of first evaluating in FP, and accepting the answer if it is exact (point on line)? This seems like it would take care of "obvious" situations such as the one above. As you say, it probably won't be consistent, and perhaps even "obviously wrong" for larger inputs (i.e. ones which are collinear using exact arithmetic on the decimal input, but for which the FP evaluation returns non-collinear due to round-off from either decimal-to-FP conversion or determinant computation). But maybe this doesn't really matter. Users should understand the limitations of finite machine arithmetic, and not expect infinite accuracy. (This would be more palatable if the topological predicates supported a precision model - which is technically feasible, and will be implemented in the future, I hope). |
For further inspiration for future work on this, this is a nice C++ implementation of Shewchuk. It has an Orient2D consistency test using perturbation (although GEOS has many more test cases). |
@dr-jts , If by that you mean DemoConsider: #include <iostream>
#include <string>
double orientation(double ax, double ay, double bx, double by, double cx, double cy)
{
double const detleft = (ax - cx) * (by - cy);
double const detright = (ay - cy) * (bx - cx);
double det = detleft - detright;
return det;
};
int main(int argc, char *argv[]) {
std::cout << orientation(std::stod(argv[1]), std::stod(argv[2]), std::stod(argv[3]), std::stod(argv[4]), std::stod(argv[5]), std::stod(argv[6])) << '\n';
return 0;
} On my ARM machine (you can get similar chaos on x86, discussed below): clang++ main.cpp -O2 && ./a.out 2 0 0 2 0.4 1.6
-2.22045e-16
g++ main.cpp -O2 && ./a.out 2 0 0 2 0.4 1.6
-2.30926e-16 In neither case det == 0. Also, note the different results from two compilers with the same (generally considered safe) flag -O2 on the same machine. The difference is GCC fsub d12, d12, d0
fsub d15, d15, d11
fsub d14, d14, d11
fsub d0, d13, d0
//...
fmul d15, d15, d12
fnmsub d0, d0, d14, d15 //<-- basically std::fma( (ax - cx), (by - cy), -detright); Clang fsub d1, d8, d12
fsub d2, d11, d0
fsub d0, d9, d0
fsub d3, d10, d12
fmul d1, d1, d2
fmul d0, d3, d0
fsub d0, d1, d0 //<-- doesn't insert FMA on O2 Even for a simpler symmetric case: g++ main.cpp -O2 && ./a.out -0.1 -0.1 0.1 0.1 0 0
8.32667e-19
g++ main.cpp -O2 && ./a.out 0.1 0.1 -0.1 -0.1 0 0
8.32667e-19
clang++ main.cpp -O2 && ./a.out 0.1 0.1 -0.1 -0.1 0 0
0 Note that this is even self-contradictory between the same three points on GCC, switching two points does not invert the sign. You'll get the same discrepancies on x86-64 with GCC between march=x86-64-v2 (what RHEL 9 is using afaik, will get you the results I get for Clang) and -march=x86-64-v3 (what they're exploring for possibly using in RHEL 10 afaik, if used with GCC on x86 it should get you the results that I get for GCC on my machine). Worse, the orientation code is a good candidate for inlining, depending on context, I could see GCC possibly deciding to put in fma in some call sites and not in others, leading to different behaviours of the same code in the build. Behaviour for all common cases of collinearitiesFor the sake of clarity, consider the following formalisation: Always: Case 1: Trivial
Case 2: Non-Trivial but power of two-denominator coordinates
Case 3: General Case (like 2 0 0 2 0.4 1.6)
Rounding determinants to zero if their absolute values are smaller than some carefully constructed error bound (i.e. filter failure) is the only heuristic that will handle all three cases well if you want a predicate that returns 0 for the double points representing some real points a, b, c with |
This PR is separated out of #1162 . It contains two changes to the orientation predicate.
The first change changes the relative error bound from currently 1e-15 to ~3.33e-16, which is proven to be sufficient in https://doi.org/10.1007/s10543-015-0574-9 by Ozaki et al. A tighter error bound can reduce the calls to the expensive exact computation in some degenerate cases (Edit: Here is a gist demonstrating this for a sequence of 100k generated near-collinear inputs https://gist.github.com/tinko92/ce44f93d8d4e4d5909310c00cf19b391 ) and has no disadvantages.
The second changes is also motivated by https://doi.org/10.1007/s10543-015-0574-9 and reduces the number of branches and instructions in the evaluation. This is more concise and may result in faster evaluation (benchmarks are in the referenced paper) but I am not currently aware of any geos-benchmarks, beside the Orientation Index micro benchmark, for which the orientation predicate is the bottle neck. The original logic that is replaced is from the function body of
REAL orient2d(pa, pb, pc)
https://www.cs.cmu.edu/afs/cs/project/quake/public/code/predicates.c , where the early returns make use of the branching in the fabs-computation, discussed in the comment preceding#define Absolute(a) ((a) >= 0.0 ? (a) : -(a))
. This reasoning is obsolete because modern compilers with enabled optimizations will compile fabs to a branchless masking of the sign bit.Edit: I did not remove the
orientation(double x)
function, which seems unused now, because I don't know if that would be considered an API change (it's not in a namespace called detail or internal or sth. like that and I don't know the geos conventions). But I can edit the commit if it is no longer needed.Edit2: Sorry for only now reading more about the jts-geos-relationship/-process. I will propose the same change to JTS, when I find time. Having said that, since patched and unpatched always return the correct orientation by deferring filter misses to the exact computation for inputs that do not go near overflow/underflow, this would not change observable behavior between JTS/Geos for the same inputs, except in computation time, so I think it classifies as an implementation detail/optimization.