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

[fix] overlay threshold for sort_by_side #1189

Merged

Conversation

barendgehrels
Copy link
Collaborator

Fixes: issue #1186

Copy link
Member

@vissarion vissarion left a 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();
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #1108

@barendgehrels barendgehrels merged commit fd209ff into boostorg:develop Sep 13, 2023
22 of 23 checks passed
@barendgehrels barendgehrels deleted the fix/issue_1186_epsilon branch September 13, 2023 18:29
@vissarion vissarion added this to the 1.84 milestone Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants