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

Julia v1.7: Multiplication broken with BigInt ranges #41517

Closed
dlfivefifty opened this issue Jul 8, 2021 · 4 comments · Fixed by #41619
Closed

Julia v1.7: Multiplication broken with BigInt ranges #41517

dlfivefifty opened this issue Jul 8, 2021 · 4 comments · Fixed by #41619
Assignees
Labels
kind:regression Regression in behavior compared to a previous version
Milestone

Comments

@dlfivefifty
Copy link
Contributor

In Julia v1.7-beta:

julia> 2 * (1:big(100)^100)
ERROR: InexactError: Int64(100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000)
Stacktrace:
 [1] Type
   @ ./gmp.jl:363 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] StepRangeLen{BigInt, BigInt, BigInt}(ref::BigInt, step::BigInt, len::BigInt, offset::Int64)
   @ Base ./range.jl:441
 [4] StepRangeLen (repeats 2 times)
   @ ./range.jl:445 [inlined]
 [5] broadcasted(#unused#::Base.Broadcast.DefaultArrayStyle{1}, #unused#::typeof(*), x::Int64, r::UnitRange{BigInt})
   @ Base.Broadcast ./broadcast.jl:1157
 [6] broadcasted
   @ ./broadcast.jl:1341 [inlined]
 [7] broadcast_preserving_zero_d
   @ ./broadcast.jl:892 [inlined]
 [8] *(A::Int64, B::UnitRange{BigInt})
   @ Base ./arraymath.jl:52
 [9] top-level scope
   @ REPL[9]:1

Previously:

julia> 2 * (1:big(100)^100)
2:2:200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

The issue is that StepRangeLen only allows Int lengths.

@dlfivefifty
Copy link
Contributor Author

The PR that changed this is #40320

One quick solution would be to only use StepRangeLen if length is an integer:

_steprangelength(a, st, n::Int) = StepRangeLen(a, st, n)
_steprangelength(a, st, n) = range(a; step=st, length=n)
broadcasted(::DefaultArrayStyle{1}, ::typeof(*), x::Number, r::AbstractRange) = _steprangelength(x*first(r), x*step(r), length(r))

@mcabbott

@JeffBezanson JeffBezanson added the kind:regression Regression in behavior compared to a previous version label Jul 8, 2021
@KristofferC KristofferC added this to the 1.7 milestone Jul 8, 2021
@mcabbott
Copy link
Contributor

mcabbott commented Jul 8, 2021

That's a pity, we wondered if there would be edge cases. Re "only use StepRangeLen if length is an Int" [surely] this would mean they can't have zero step. Maybe nobody would ever want both at once?

Do many other things assume length(::AbstractVector)::Int? Arrays too long to fit in 64 bits are pretty big. On Julia 1.6:

julia> ans'
1×100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 adjoint(::StepRange{BigInt, BigInt}) with eltype BigInt:
Error showing value of type Adjoint{BigInt, StepRange{BigInt, BigInt}}:
ERROR: InexactError: Int64(100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000)
Stacktrace:
  [1] Type
    @ ./gmp.jl:362 [inlined]
  [2] convert
    @ ./number.jl:7 [inlined]
  [3] to_index
    @ ./indices.jl:292 [inlined]

@dlfivefifty
Copy link
Contributor Author

Note the related commit that fixed many of these issues: #37741

I ran into this particular problem in InfiniteArrays.jl, where the length of the array is infinite. This has become an accepted use case:

https://github.com/JuliaLang/julia/blob/3eefaf0a52d1f537c512282a3027ead8e5e4f44e/test/testhelpers/InfiniteArrays.jl

I believe I can work around this particular issue as-is, albeit _steprangelength function would be slightly cleaner. The other solution would be to add a type parameter for the length in StepRangeLen, but that might count as a breaking change.

Though I have to say I don't understand why you didn't change Base.range_start_step_length to return a StepRangeLen?

@JeffBezanson
Copy link
Sponsor Member

JeffBezanson commented Jul 15, 2021

From triage:

  • It seems the only way to fully fix this is to add a type parameter to StepRangeLen for the type of the length. That is generally not breaking, but it can turn a concrete type into an abstract type so some kinds of issues are possible. (@vtjnash is exploring this)
  • The other option is to revert the change on the release branch, but we are not so keen on that either.

vtjnash added a commit that referenced this issue Jul 15, 2021
Also be more careful about using additive identity instead of
multiplicitive, and be more consistent about types in a few places.

Fixes #41517
@vtjnash vtjnash self-assigned this Jul 15, 2021
vtjnash added a commit that referenced this issue Jul 16, 2021
Also be more careful about using additive identity instead of
multiplicative, and be more consistent about types in a few places.

Fixes #41517
vtjnash added a commit that referenced this issue Jul 21, 2021
Also be more careful about using additive identity instead of
multiplicative, and be more consistent about types in a few places.

Fixes #41517
vtjnash added a commit that referenced this issue Jul 23, 2021
Allows creating these ranges for any type of integer lengths.

Also need to be careful about using additive identity instead of
multiplicative, and be even more consistent now about types in a
few places.

Fixes #41517
KristofferC pushed a commit that referenced this issue Jul 26, 2021
Allows creating these ranges for any type of integer lengths.

Also need to be careful about using additive identity instead of
multiplicative, and be even more consistent now about types in a
few places.

Fixes #41517

(cherry picked from commit 4f77aba)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants