-
Notifications
You must be signed in to change notification settings - Fork 44
Fix QCheck{,2}.Gen.float distribution #350
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
Conversation
|
I think it's great. It seems like I wonder if the shrinker can also operate on nans by going back to the bitwise representation? |
|
Looks great, nothing to add on the generation side, thank you @jmid! |
|
Thanks both! 🙏
Yep, the PR fixes both. QCheck{,2} is my (not particularly clear 😅 ) way of writing QCheck and QCheck2.
I hadn't considered that! 🤔 Finally, the CI was failing on Windows, which has an annoying habit of printing a leading exponent zero in floating point numbers (such as --- a/_build/default/test/core/QCheck2_expect_test.expected
+++ b/_build/default/test/core/QCheck2_expect_test.exe.output
@@ -376,13 +376,13 @@ Test float < 1.0 failed (450 shrink steps):
Test float <= 1e-10 failed (482 shrink steps):
-1.00003747936e-10
+1.00003747936e-010
--- Failure --------------------------------------------------------------------
Test float >= -1e-10 failed (661 shrink steps):
--1.00001604579e-10
+-1.00001604579e-010
--- Failure --------------------------------------------------------------------5e65bfe lifts the workaround from #326 to apply to both |
This may be equivalent, but I would rather consider shrinking on the exponent and mantissa? |
Yep, that's also my approach 👍 The WIP however gives up on let float x yield =
if Float.is_infinite x || Float.is_nan x then () else
... |
Nice! I guess it might be interesting to have a shrinker for NaNs in some specialized cases, but this is a highly advanced feature IMO. |
|
I didn't mean to derail with the NaN comment. Tbh a very simple shrinker could be that we pick a canonical NaN value and just shrink any other NaN to it (which is useful info when a bug is triggered by any NaN) |
I accidentally discovered that the following test ("all floats are smaller or equal than 1e10") passed:
Upon further inspection, it turns out$2^{-21}$ and $2^{22}$ (see stats output in 0f48fc2) which is far from the $2^{-1022}$ and $2^{1023}$ bounds described in https://en.wikipedia.org/wiki/Double-precision_floating-point_format. This is unfortunate, as a limited distribution means that bugs outside the above range may have flown under the rader.
QCheck{,2}.Gen.floatusesexpto generate floating point numbers and has an output with an exponent betweenThis PR thus replaces the existing generator with one based on
Int64.float_of_bitswhich is fed 64 random bits.The latter is achieved by stitching together enough 30-bit
Random.State.bitschunks together.OCaml 4.14+ does offer
Random.State.bits64but we are already maintaining 4 expect test outputs ({ocaml4,ocaml5} x {32-bit,64-bit}) and I don't feel like extending the first set of that matrix with an OCaml 4.8.-4.13. entry. I instead suggest replacing the stitching once we push the lower bound to OCaml 4.14.The resulting generator should have an equal chance of all 64-bit patterns - not to be confused with an equal chance of all 64-bit floating point numbers, as, e.g., multiple bit-patterns may represent
nan.I've added a
collectstatistic illustrating that the replacement generator may output bothnanand subnormal floating point numbers for now:If the interpretation differs across architectures (AMD64,ARM64,s390x,RISC-V,PPC64) we may have to remove this expect test again. I found it nice to confirm a difference with the previous distribution with all
FP_normalclassification.Finally, the expect tests illustrate the need for a
QCheckfloatshrinker. I was working on such a thing when I discovered the above limitation.The PR is structured as follows:
classify_floatstatPolite ping to @c-cube and to @rmonat who has been using the
QCheck2.Gen.floatgenerator recently 🙂