Don't assume NRoot[Real] is perfect #1173
Conversation
| implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs) | ||
| forAll { (x0: Real, k: Sized[Int, _1, _10]) => |
There was a problem hiding this comment.
I'm not really sure if this is any nicer, but it is a bit more explicit :)
| implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs) | |
| forAll { (x0: Real, k: Sized[Int, _1, _10]) => | |
| forAll(realFromLongs, arbitrary[Sized[Int, _1, _10]]) { (x0, k) => |
There was a problem hiding this comment.
Ah, this will fix the compile errors I reckon.
There was a problem hiding this comment.
Yeah, this worked in 3 but not in 2 😮 -- implicit resolution must have changed in 3.
I split these tests into a separate suite so that it's more clear.
There was a problem hiding this comment.
I think if we explicitly pass the generator, then we can keep them in the same file.
There was a problem hiding this comment.
Happy to go that route if that's standard here - I felt kind of bad writing arbitrary[Sized[Int, _1, _10]] 3 times though 🤷♂️
There was a problem hiding this comment.
I'm not entirely sure what Sized is, but you may be able to use Gen.chooseNum or something to achieve the same result. I think it's worth it to keep the tests together.
There was a problem hiding this comment.
I'm struggling to find a solution that uses realFromLongs in the derivation of Arbitrary[NonZero[Real]] for this case that is compatible between scala 2 and 3 but keeps everything in one suite. It seems like where I'd previously have used an implicit def in scala 2, I should be using a given in scala 3. I suppose two suites in one file would be doable here too, but that's not far off from what's already in the PR.
I pulled in that test here because it seemed like it was susceptible to the same problem in the issue. What do you think the path forward should be?
There was a problem hiding this comment.
I'm not sure I understand what the Scala 2/3 issue is here, since implicit def should also work on Scala 3. Are you saying that Arbitrary[NonZero[Real]] is not using the implicit val realArb: Arbitrary[Real] at the top of that file?
There was a problem hiding this comment.
Ok so I read the compiler output again and it's not a compat issue, I'm just not sure how to provide implicit evidence properly. Sorry about the confusion.
Here's the current NonZero instance:
implicit def nonZeroSpireImplicit[A: Signed: AdditiveGroup: Arbitrary]: Arbitrary[NonZero[A]] =
Arbitrary(arbitrary[A].filter(_.signum != 0).map(NonZero(_)))I want the instance of Arbitrary[A] used here to be realFromLongs, so I updated it to:
implicit def nonZeroSpireImplicit[A: Signed: AdditiveGroup](implicit arbA: Arbitrary[A]): Arbitrary[NonZero[A]] =
Arbitrary(arbA.arbitrary.filter(_.signum != 0).map(NonZero(_)))I was hoping to pass in realFromLongs explicitly, but in doing so I can't find evidence for Signed[Real].
There was a problem hiding this comment.
Aha, gotcha, no worries :) IIRC before you changed it the signature was like this:
implicit def nonZeroSpireImplicit[A: Signed: AdditiveGroup: Arbitrary]: Arbitrary[NonZero[A]]You can then explicitly pass in realFromLongs with something like this:
nonZeroSpireImplicit[Real](implicitly, implicitly, Arbitrary(realFromLongs))| property("x.pow(3) = x * x * x") { | ||
| forAll { (x: Real) => | ||
| x.pow(2) == x * x | ||
| x.pow(3) == x * x * x |
| // sample1("sample1 id")(x => x) | ||
| // sample1("sample1 negate")(x => -x) | ||
| // sample1("sample1 +")(x => x + x) | ||
| // sample1("sample1 *")(x => x * x) | ||
| // sample1("sample1 sqrt")(_.sqrt()) | ||
| // sample1("sample1 pow(2)")(_.pow(2)) |
There was a problem hiding this comment.
I wonder what this was. It's been commented for nearly a decade now it seems.
There was a problem hiding this comment.
Your guess is as good as mine. It seemed like a unused test that I couldn't really decipher.
Fixes #1106.
Open to other suggestions here!