-
Notifications
You must be signed in to change notification settings - Fork 228
Feature/robust predicates #617
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
Feature/robust predicates #617
Conversation
vissarion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I am ok with the math, the computations seem ok too. However, the strategy needs some cleaning/revision.
| // Unit Test | ||
|
|
||
| // Copyright (c) 2019 Tinko Bartels, Berlin, Germany. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could acknowledge GSoC here if you like. See for example https://github.com/boostorg/geometry/blob/develop/include/boost/geometry/formulas/karney_direct.hpp#L5
| left of segment (>0), right of segment (< 0), on segment (0). | ||
| \ingroup strategies | ||
| \tparam CalculationType \tparam_calculation (numeric_limits<ct>::epsilon() and numeric_limits<ct>::digits must be supported for calculation type ct) | ||
| \tparam robustness Number that determines maximum precision. Values from 0 to 2 may make the calculation terminate faster for inputs that may require higher precision to ensure correctness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not very clear. Do you want to say values from 0 to 3, where 0 is the fastest and less accurate and 3 the slowest and most accurate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, I'll reconsider the wording there. It is a little hard to say it in one sentence.
By default (robustness = 3), the strategy will compute up to four successively more precise approximations of the true side_value (with the fourth being guaranteed to be correct) and check for each approximation whether its sign can already be guaranteed using relative error bounds. If that is the case, the remaining approximations (if any) will not be computed and the current approximation will be returned.
If the robustness parameter is set to 0, it will return the first approximation without any test against the error bounds, if it is set to 1, it will compute the first approximation, test it against the error bound and either return if the precision is sufficient or compute the second approximation. With robustness = 2, up to three approximations will be computed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the technical details are not needed here. Hence, saying something like 0 is the fastest and less accurate and 3 the slowest and most accurate, is fine.
| //! \brief Computes double the signed area of the CCW triangle p1, p2, p | ||
| template | ||
| < | ||
| typename CoordinateType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused template parameter
| typename P2, | ||
| typename P | ||
| > | ||
| static inline int apply(P1 const& p1, P2 const& p2, P const& p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is too generic. What is the case where p1 and p2 are of different types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine such a case, however, my choice to be that generic was motivated by side_by_triangle having the same interface and I wanted to be as compatible with it as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the same interface is OK.
| typedef typename boost::mpl::if_c | ||
| < | ||
| boost::is_void<CalculationType>::type::value, | ||
| typename select_most_precise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a select_most_precise with 3 arguments
| typedef typename coordinate_type<P2>::type coordinate_type2; | ||
| typedef typename coordinate_type<P>::type coordinate_type3; | ||
|
|
||
| typedef typename boost::mpl::if_c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is implemented in utilities as select_calculation_type_alt
|
|
||
|
|
||
| promoted_type sv = | ||
| side_value<coordinate_type, promoted_type>(p1, p2, p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coordinate_type here is unused
…on in side_robust
…eature/robust_predicates
| template | ||
| < | ||
| typename CalculationType = void, | ||
| int robustness = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tinko, thanks a lot for your contributions!
Can you spell the robustness template parameter as Robustness and maybe make it an std::size_t
| typename P2, | ||
| typename P | ||
| > | ||
| static inline int apply(P1 const& p1, P2 const& p2, P const& p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the same interface is OK.
vissarion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tinko thanks for the revision, I am ok with merging now.
| left of segment (>0), right of segment (< 0), on segment (0). | ||
| \ingroup strategies | ||
| \tparam CalculationType \tparam_calculation (numeric_limits<ct>::epsilon() and numeric_limits<ct>::digits must be supported for calculation type ct) | ||
| \tparam robustness Number that determines maximum precision. Values from 0 to 2 may make the calculation terminate faster for inputs that may require higher precision to ensure correctness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the technical details are not needed here. Hence, saying something like 0 is the fastest and less accurate and 3 the slowest and most accurate, is fine.
|
Thanks for this PR! I'm ok with merging after the issues above are addressed. I'd like to be explicit regarding one thing though. This PR depends on C++11. After merging it the library will partially depend on newer standard. I'm ok with that since we plan to move to C++11 anyway, but I'd like to get explicit confirmation from you @barendgehrels @vissarion. |
|
Thanks Tinko! |
Introduction
This is the first (and most simple and self-contained) of three branches that I created during the GSoC 2019 that has now concluded. It provides two strategies for 2d cartesian points:
The strategy classes come with documentation and tests, currently in the extension-directories.
Rationale
The new side-strategy can be used where side_by_triangle is used and may help in situations where computations break down because of incorrect results in side_by_triangle.
The in_circle-strategy will be required by algorithms for the construction of Delaunay triangulations such as the one I will propose in a later pull request. It may also be of use in constructing an adaptive, robust side-strategy for circular segments, which were recently proposed by @meastp in the Gitter room and generally useful on its own for users.
Implementation details
Both strategies are based on evaluating certain determinants (like the existing side_by_triangle), They are robust in the sense that they return correct results even on inputs where direct float arithmetics would fail due to precision issues, such as side of POINT(1, 1) with respect to the segment of POINT( 1e-20, 0) and POINT(1e20, 1e20). They are adaptive in the sense that for inputs where direct float arithmetic can provably produce correct results (like in all non-degenerate inputs), the computation terminates early to save time. This is done using analytically computed error bounds. The algorithm is intended for floating-point parameters but returns correct results for integers that can be translated to the calculation_type without loss of precision. It is not robust with respect to inputs that cause an overflow in the computation of interim results.
All of the math behind this is explained in the papers linked here https://www.cs.cmu.edu/~quake/robust.html. The details are implemented almost directly as explained in the paper. All of the implementation details (the mathematics) are in the file
include/boost/geometry/extensions/triangulation/strategies/cartesian/detail/precise_math.hpp
The robustness-parameter is my own addition and can be used to limit the maximum precision. If 0 is provided, the predicates are comparatively fast.