-
Notifications
You must be signed in to change notification settings - Fork 93
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
Improve low discrepancy sequences #2654
Conversation
mbaudin47
commented
May 12, 2024
•
edited
edited
- Improve unit test of Halton sequence
- Improve unit test of Faure sequence
- Improve unit test of Sobol' sequence
- Closes The Faure sequence is wrong #2653
40ac801
to
f1067d7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2654 +/- ##
=======================================
Coverage 78.47% 78.48%
=======================================
Files 840 840
Lines 96641 96641
=======================================
+ Hits 75836 75844 +8
+ Misses 20805 20797 -8 ☔ View full report in Codecov by Sentry. |
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.
Very nice work! Thanks for digging into this not-so-easy piece of code. I would be reassured if you checked the values for larger sequences.
expected3D[5] = Point({2.0 / 9.0, 8.0 / 9.0, 5.0 / 9.0}); | ||
expected3D[6] = Point({5.0 / 9.0, 2.0 / 9.0, 8.0 / 9.0}); | ||
expected3D[7] = Point({8.0 / 9.0, 5.0 / 9.0, 2.0 / 9.0}); | ||
FaureSequence sequence3D(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.
Have you checked the values for larger samples, even if these values don't have to be checked in the test? This is just the beginning of the sequence and surprises can pop up for larger values...
|
good to go after the first Regis comment is addressed |