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

Eq[Polynomial[C]] throws ArrayIndexOutOfBoundsException for negative exponents #756

Open
LukaJCB opened this issue Jan 9, 2019 · 7 comments

Comments

@LukaJCB
Copy link
Member

LukaJCB commented Jan 9, 2019

Reproduce:

scala> def x: Polynomial[Int] = PolySparse(List(Term(-1642198702, -1)))
x: spire.math.Polynomial[Int]
scala> x === x
java.lang.ArrayIndexOutOfBoundsException: -1
  at scala.runtime.ScalaRunTime$.array_update(ScalaRunTime.scala:76)
  at spire.math.poly.PolySparse.coeffsArray(PolySparse.scala:54)
  at spire.math.PolynomialEq.eqv(Polynomial.scala:556)
  at spire.math.PolynomialEq.eqv$(Polynomial.scala:555)
  at spire.math.PolynomialInstances0$$anon$14.eqv(Polynomial.scala:568)
  ... 36 elided
@denisrosset
Copy link
Collaborator

I'm not sure polynomials were designed to accept negative exponents. For example, the parse method on the companion object does not support negative exponents (see the termRe regexp).

@tixxit: can you comment on this?

@tixxit
Copy link
Contributor

tixxit commented Jan 16, 2019

They were not - I think it is probably a bug that we don't throw an exception during construction in this case and we should fix that bug.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 16, 2019

Then we must also fix this line right here, which generates random exponents that could all be negative :)
https://github.com/non/spire/blob/master/laws/src/main/scala/spire/laws/gen.scala#L100

@tixxit
Copy link
Contributor

tixxit commented Jan 16, 2019

😬

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 16, 2019

Submitted a quick fix for that particular problem.

@kschwarz1116
Copy link
Contributor

If polynomials aren't supposed to have negative exponents, should Term have UInt for exponent instead of Int?

@armanbilge
Copy link
Member

@kschwarz1116 that's a good question. I wonder if UInt interacts poorly with specialization, and so would be boxed, which is why Int is preferable for performance. But I'm not sure about that without looking at bytecode.

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

No branches or pull requests

6 participants