added deterministic option #550
Conversation
| #else | ||
| std::sort(starts.begin(), starts.end(), cmp); | ||
| std::stable_sort(starts.begin(), starts.end(), cmp); |
There was a problem hiding this comment.
Do these stable sorts make much difference?
There was a problem hiding this comment.
probably not, this is just in case. the performance of stable sort is not a lot different than sort from my testing
(FYI: rust uses stable sort by default, and the c++ sort is named unstable sort in rust and we will only use it when profile shows it is beneficial)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #550 +/- ##
==========================================
- Coverage 91.23% 91.13% -0.10%
==========================================
Files 35 35
Lines 4553 4547 -6
==========================================
- Hits 4154 4144 -10
- Misses 399 403 +4
☔ View full report in Codecov by Sentry. |
| FaceAreaVolume({halfedge_, vertPos_, precision_}), | ||
| thrust::make_pair(0.0f, 0.0f), SumPair()); | ||
| return {areaVolume.first, areaVolume.second}; | ||
| // Kahan summation |
There was a problem hiding this comment.
This is cool - I hadn't heard of this before.
There was a problem hiding this comment.
Same here, I just know this after searching for numerically stable summation algorithm.
* added deterministic option * fix formatting * address some comments * use kahan summation * revert DoesOverlap changes * remove old code * formatting
This disables some racy optimizations when user wants deterministic output. We should now consider non-deterministic output when deterministic option is enabled a bug.
The area and volume calculation now uses Kahan summation, which is numerically stable.
Also, it turns out parallel compose will somehow cause non-determinism. Considering we already do parallel for inside compose, I just disabled doing multiple compose in parallel and rewrote it in a simpler way.
Fixes #545.