Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't assume NRoot[Real] is perfect #1173

Merged
merged 10 commits into from
May 21, 2022
Merged

Don't assume NRoot[Real] is perfect #1173

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3564bdc
only test against longs that don't bust precision
May 13, 2022
8097722
test cube
May 13, 2022
74326e5
get rid of unecessary comments
May 13, 2022
9f8f858
add root clarification
May 16, 2022
dcd4719
Add missing dash
kschwarz1116 May 16, 2022
8c0260a
split real root testing into separate suite
May 16, 2022
20edc9b
Merge branch 'fix-nroot' of github.com:kschwarz1116/spire into fix-nroot
May 16, 2022
cf4c541
revert fractionalsuite change, use local implicits
May 19, 2022
802c1f2
scalafmt
May 19, 2022
6458083
make it work in scala2
May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,9 @@ import Real.{sin, cos}
// will return Real(1) no matter what value is provided
def circle(a: Real): Real = sqrt(cos(a).pow(2) + sin(a).pow(2))
```
Keep in mind that precision of roots is not perfectly accurate. See
[Irrational and Transcendental type classes](#irrational-and-transcendental-type-classes)
for more information.

One interesting consequence of the design of computable real numbers
is non-continuous operations (such as sign tests, comparisons, and
Expand Down
15 changes: 9 additions & 6 deletions laws/src/main/scala/spire/laws/gen.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ object gen {
lazy val natural: Gen[Natural] =
Gen.oneOf(arbitrary[Long].map(n => Natural(n & Long.MaxValue)), arbitrary[BigInt].map(n => Natural(n.abs)))

lazy val rational: Gen[Rational] = {
val rationalFromLongs: Gen[Rational] =
for {
n <- arbitrary[Long]
d <- arbitrary[Long].map(n => if (n == 0) 1L else n)
} yield Rational(n, d)
lazy val rationalFromLongs: Gen[Rational] =
for {
n <- arbitrary[Long]
d <- arbitrary[Long].map(n => if (n == 0) 1L else n)
} yield Rational(n, d)

lazy val rational: Gen[Rational] = {
val rationalFromSafeLongs: Gen[Rational] =
for {
n <- safeLong
Expand Down Expand Up @@ -106,6 +106,9 @@ object gen {
lazy val real: Gen[Real] =
rational.map(Real(_))

lazy val realFromLongs: Gen[Real] =
rationalFromLongs.map(Real(_))

lazy val sign: Gen[Sign] =
Gen.oneOf(Signed.Positive, Signed.Zero, Signed.Negative)

Expand Down
29 changes: 6 additions & 23 deletions tests/shared/src/test/scala/spire/math/RealScalaCheckSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ package math

import spire.implicits._
import spire.laws.arb.{rational, real}
import spire.laws.gen.realFromLongs

import ArbitrarySupport._
import Ordinal._
import org.scalacheck.Arbitrary
import org.scalacheck.Prop._

class RealScalaCheckSuite extends munit.ScalaCheckSuite {
Expand Down Expand Up @@ -112,25 +114,28 @@ class RealScalaCheckSuite extends munit.ScalaCheckSuite {

property("x.pow(3) = x * x * x") {
forAll { (x: Real) =>
x.pow(2) == x * x
x.pow(3) == x * x * x
Comment on lines 115 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, thank you! :)

}
}

property("x.pow(k).nroot(k) = x") {
implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs)
forAll { (x0: Real, k: Sized[Int, _1, _10]) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if this is any nicer, but it is a bit more explicit :)

Suggested change
implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs)
forAll { (x0: Real, k: Sized[Int, _1, _10]) =>
forAll(realFromLongs, arbitrary[Sized[Int, _1, _10]]) { (x0, k) =>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this will fix the compile errors I reckon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we explicitly pass the generator, then we can keep them in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to go that route if that's standard here - I felt kind of bad writing arbitrary[Sized[Int, _1, _10]] 3 times though 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@armanbilge armanbilge May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Member

@armanbilge armanbilge May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

val x = x0.abs
x.pow(k.num).nroot(k.num) == x
}
}

property("x.nroot(k).pow(k) = x") {
implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs)
forAll { (x0: Real, k: Sized[Int, _1, _10]) =>
val x = x0.abs
x.nroot(k.num).pow(k.num) == x
}
}

property("x.nroot(-k).pow(-k) = x") {
implicit val realArb: Arbitrary[Real] = Arbitrary(realFromLongs)
forAll { (x0: NonZero[Real], k: Sized[Int, _1, _10]) =>
val x = x0.num.abs
x.nroot(-k.num).pow(-k.num) == x
Expand Down Expand Up @@ -196,28 +201,6 @@ class RealScalaCheckSuite extends munit.ScalaCheckSuite {
}
}

// def sample1(name: String)(f: Real => Real): Unit = {
// property(name) {
// forAll { (x0: Rational, i0: Byte, j0: Byte) =>
// val x = f(Real(x0.abs))
// val i = (i0 & 0xff) % 250 + 1
// val j = (j0 & 0xff) % 250 + 1
// val (k1, k2) = if (i <= j) (i, j) else (j, i)
// val v1 = x(k1)
// val v2 = x(k2)
// val v3 = Real.roundUp(Rational(v2, SafeLong(2).pow(k2 - k1)))
// v1 == v3
// }
// }
// }

// 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))
Comment on lines -214 to -219
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what this was. It's been commented for nearly a decade now it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your guess is as good as mine. It seemed like a unused test that I couldn't really decipher.


def arcSample(f: Rational => Rational)(g: Double => Double, h: Real => Real): String =
(-8 to 8).map { i =>
val x = Real(f(Rational(i)))
Expand Down