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.

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

RealScalaCheckSuite.x.pow(k).nroot(k) = x failed #1106

Closed
armanbilge opened this issue Nov 3, 2021 · 3 comments · Fixed by #1173
Closed

RealScalaCheckSuite.x.pow(k).nroot(k) = x failed #1106

armanbilge opened this issue Nov 3, 2021 · 3 comments · Fixed by #1173
Labels

Comments

@armanbilge
Copy link
Member

https://github.com/typelevel/spire/runs/4087917370?check_suite_focus=true#step:8:1039

==> X spire.math.RealScalaCheckSuite.x.pow(k).nroot(k) = x  0.049s munit.FailException: /home/runner/work/spire/spire/tests/shared/src/test/scala/spire/math/RealScalaCheckSuite.scala:124
123:    }
124:  }
125:

Failing seed: IhNYEXMVtFv8DyJdCDjhw3OngO0cs8m_RcJEQuIvjaG=
You can reproduce this failure by adding the following override to your suite:

  override val scalaCheckInitialSeed = "IhNYEXMVtFv8DyJdCDjhw3OngO0cs8m_RcJEQuIvjaG="

Falsified after 9 passed tests.
> ARG_0: -2759766405657757/1532495540865888858358347027150309183618739122183602176
> ARG_1: Sized(6)
    at munit.Assertions.fail(Assertions.scala:283)
    at munit.Assertions.fail$(Assertions.scala:15)
    at munit.FunSuite.fail(FunSuite.scala:11)
    at munit.ScalaCheckSuite.propToTry$$anonfun$1(ScalaCheckSuite.scala:104)
    at scala.util.Try$.apply(Try.scala:210)
    at munit.ScalaCheckSuite.propToTry(ScalaCheckSuite.scala:104)
    at munit.ScalaCheckSuite.$init$$$anonfun$2$$anonfun$1$$anonfun$1(ScalaCheckSuite.scala:46)
    at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:439)
    at munit.Suite$$anon$1.execute(Suite.scala:26)
    at scala.concurrent.impl.Promise$Transformation.submitWithValue(Promise.scala:393)
    at scala.concurrent.impl.Promise$DefaultPromise.submitWithValue(Promise.scala:302)
    at scala.concurrent.impl.Promise$DefaultPromise.dispatchOrAddCallbacks(Promise.scala:276)
    at scala.concurrent.impl.Promise$DefaultPromise.transform(Promise.scala:128)
    at munit.internal.FutureCompat$ExtensionFuture.transformCompat(FutureCompat.scala:16)
    at munit.ScalaCheckSuite.$init$$$anonfun$3$$anonfun$2(ScalaCheckSuite.scala:48)
    at munit.GenericTest.withBodyMap$$anonfun$1(GenericTest.scala:33)
    at munit.MUnitRunner.$anonfun$8(MUnitRunner.scala:296)
@armanbilge armanbilge added the bug label Nov 3, 2021
@kschwarz1116
Copy link
Contributor

It appears to be acting on Rational under the hood:

  test("rational val") {
    import spire.math.Fractional.RationalIsFractional
    val x =  Rational(2759766405657757L, SafeLong(BigInt("1532495540865888858358347027150309183618739122183602176")))

    assertEquals(x.pow(3).nroot(3), x) // Rational fails at 3, Real fails at 6
  }

This equality fails for sqrt too if we look large enough:

  test("rational val") {
    import spire.math.Fractional.RationalIsFractional
    val x =  Rational(2759766405657757L, SafeLong(BigInt(2).pow(1000)))
    assertEquals(x.pow(2).nroot(2), x) // fails
  }

nroot on Rational has a comment that it occurs at Double precision. This is clearly pushing the boundary. I think reasonable solutions include attempting to up the nroot precision (at cost of performance), or just updating the test to only look at rationalFromLongs.
Docs could be updated to be more accurate on this too.

@armanbilge
Copy link
Member Author

Thanks for looking into this! It seems strange to make that NRoot instance available by default since it is only approximate. I wonder if it should be moved out of implicit scope so that it has to be imported explicitly, do you have any opinions?

Testing with the rationalFromLongs generator sounds good to me, as does updating the docs. PRs welcome! :)

@kschwarz1116
Copy link
Contributor

It wouldn't be the first time someone suggested NRoot[Rational] looks suspicious. I'm for making it an explicit import.

I'll try getting around to a PR for these changes in the next couple days!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants