qcheck icon indicating copy to clipboard operation
qcheck copied to clipboard

Provide `int32` and `int64` equivalent functions of `int` and `float`

Open sir4ur0n opened this issue 4 years ago • 9 comments

I could not find a way in QCheck to generate an int32 or int64 within a range, similar to range_int/-- for int and range_float/--. for float.

Is it currently possible? Otherwise, what do you think about adding those?

I guess this would also apply to functions like xxx_bound, pos_xxx, neg_xxx?

Arguably this would not be useful for the small_xxx family of functions as one could just generate int and convert them, but it may be more consistent to add those too (I don't have any opinion)

While this hasn't impacted me yet, I see that even float doesn't always have an equivalent function of int either

This is a messy issue, talking about int64, int32, float... Sorry :sweat_smile:

sir4ur0n avatar Mar 12 '21 14:03 sir4ur0n

It would be useful, but I have little time to work on qcheck currently. I'd review/merge PRs of course :).

c-cube avatar Mar 12 '21 15:03 c-cube

Thank you @c-cube

I just want to clarify that I am not coming with my Santa list and waiting for you to implement it all :sweat_smile: I'm first and foremost opening these issues to discuss adding such features to QCheck (and sometimes discover it already (partially) existed, like the runner error messages :grin: )

If/once you agree on a design, anyone (you, me, my teammates, or anyone reading this, in no particular order :grin: ) can contribute said feature. I hope I will be able to contribute some (modest) ones (or propose a rough draft when I have already implemented it for my needs, like the generator that retries in #104 )

sir4ur0n avatar Mar 12 '21 15:03 sir4ur0n

I agree such generators would be a nice addition (also for consistency). It should be relatively straightforward to add them if you study the existing int implementations in, e.g., https://github.com/c-cube/qcheck/blob/master/src/core/QCheck.ml#L158

Previously I've contributed a couple of such extensions as I required them myself. As such I see QCheck developing very much "by need" 🙂 If nothing else opening an issue is valuable to document the shortcoming to others 🤷‍♂️

jmid avatar Mar 12 '21 16:03 jmid

I just want to clarify that I am not coming with my Santa list and waiting for you to implement it all sweat_smile

good, because it's march, there's still > 9 months to go :wink: .

As such I see QCheck developing very much "by need" slightly_smiling_face

Yes, that's how I see it too. This way it's sustainable and only useful stuff should be added…

c-cube avatar Mar 12 '21 16:03 c-cube

It should be relatively straightforward to add them if you study the existing int implementations in, e.g.,

It is not 100% straightforward because some inner function uses Random.State.bytes which generates an int. Besides that, this looked pretty straightforward indeed. If one has an idea how to circumvent this, maybe I can give it a shot

sir4ur0n avatar Mar 15 '21 17:03 sir4ur0n

For, e.g., i64_bound and i64_range it should be possible to mimic the int versions, shouldn't it? I can see pint uses RS.bits by glueing chunks of 30 random bits together. I believe we inherited that approach from https://github.com/ocaml/ocaml/blob/trunk/stdlib/random.ml#L112

In November I had my fingers in int_bound and int_range. See https://github.com/c-cube/qcheck/issues/100 (also for an explanation of the current version - and a few pitfalls)

jmid avatar Mar 15 '21 20:03 jmid

While working on integrated shrinking I think I noticed something wrong regarding Gen.ui32/64: they are signed :thinking:

Gen.generate ~n:10 (Gen.ui32) ;;
- : int32 list =
[-573812011l; -1494688812l; -1223342577l; 732077606l; 1865555234l; 141484367l;
 172385682l; 558309696l; 83912486l; -574950022l]

It is all the more surprising that the arbitraries int32/64 rely on those "unsigned" generators...

Have I missed something? I expected "unsigned" to mean "positive"

Indeed as the current implementation generates 32- or 64-bits random strings, the sign bit is random too, hence negative numbers.

As @jmid hinted I think we can generalize the current implementation of pint by taking as parameter a bit size. Then all of positive, negative, and signed numbers (int/int32/int64) could reuse this single generator by passing either 32, 64 or Sys.int_size (minus 1 for positive- or negative-only numbers).

As the current PR https://github.com/c-cube/qcheck/pull/109 touches a lot of code, I think I won't touch this for now, but this would be great to clarify this afterwards

Edit: I wonder if the "unsigned" thingy for generators comes from the fact that Int32/64.of_string functions read "unsigned" strings? But the resulting int32/64 number is indeed signed, so this is confusing...

sir4ur0n avatar Apr 28 '21 14:04 sir4ur0n

@Sir4ur0n Surprising indeed! Also see #90 - I can see I'm assigned but I unfortunately have my hands full in a new job.

jmid avatar Apr 28 '21 16:04 jmid

:facepalm: I didn't see this issue, my bad, thank you :sweat_smile:

sir4ur0n avatar Apr 28 '21 19:04 sir4ur0n