-
Notifications
You must be signed in to change notification settings - Fork 216
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
[fix] overlay threshold for sort_by_side #1189
[fix] overlay threshold for sort_by_side #1189
Conversation
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!
It would help a lot if you comment on the geometric intuition of the value 100 that is used as threshold.
Regarding #1183 is there some other combination of values that fixes that or you believe it is of different nature?
@@ -325,7 +325,7 @@ public : | |||
double | |||
>::type; | |||
|
|||
ct_type const tolerance = 1000000000; | |||
static auto const tolerance = common_approximately_equals_epsilon<ct_type>::value(); |
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 the actual change right?
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.
Indeed!
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'll document the 100
later. It's not new, it's moved.
It is recently (earlier this year) adapted from a larger value to this value, considering another issue and all current tests.
It is used for clustering turns, and now also for sort by side. In both cases the exact value is not that important, but it should not be 1 (for floating point comparisons) and not be too large (obviously). The tolerance which was now replaced was very large, I don't remember exactly why.
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.
See also #1108
Fixes: issue boostorg#1186
0d5f1a0
to
1a0c287
Compare
Fixes: issue #1186