Skip to content

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Jul 22, 2016

I believe this is a bug, since it's very strange that passing an Int32 resulted in a ReshapedArray while passing an Int resulted in an Array.

This synchronizes DimOrInd with other index-related types, all of which use Int. This will also affect packages like DistributedArrays, which also assume Int.

@tkelman
Copy link
Contributor

tkelman commented Jul 22, 2016

sure we don't have any tests that use this? this is one possible decision to make on that issue but I wouldn't consider turning it into an error a "fix"

Before, the definition of `reshape` that returns a `ReshapedArray` accepted
any integer, but the Array->Array definition only accepted `Int`, so passing a
different type of integer strangely resulted in a different type of array.
@JeffBezanson
Copy link
Member Author

It was always an error before. And I certainly hope we're not testing that passing a non-Int to reshape results in a ReshapedArray instead of an Array.

@tkelman
Copy link
Contributor

tkelman commented Jul 22, 2016

the issue (and at least one duplicate of it) is saying "it would be nice if this weren't an error." I agree the inconsistency here is good to fix, but a looser signature would be more convenient

@JeffBezanson
Copy link
Member Author

For that we would have to change a large number of places, plus many packages.

typealias AbstractVecOrMat{T} Union{AbstractVector{T}, AbstractMatrix{T}}
typealias RangeIndex Union{Int, Range{Int}, AbstractUnitRange{Int}, Colon}
typealias DimOrInd Union{Integer, AbstractUnitRange}
typealias IntOrInd Union{Int, AbstractUnitRange}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make it AbstractUnitRange{Int}?

@timholy
Copy link
Member

timholy commented Jul 22, 2016

I like this solution. There are a lot of inconsistencies though. Here are some other examples where we specify the desired size of something using Integer:

julia/base/sysimg.jl

Lines 75 to 85 in f331e02

(::Type{Array{T}}){T}(m::Integer) = Array{T,1}(Int(m))
(::Type{Array{T}}){T}(m::Integer, n::Integer) = Array{T,2}(Int(m), Int(n))
(::Type{Array{T}}){T}(m::Integer, n::Integer, o::Integer) = Array{T,3}(Int(m), Int(n), Int(o))
(::Type{Array{T}}){T}(d::Integer...) = Array{T}(convert(Tuple{Vararg{Int}}, d))
(::Type{Vector})() = Array{Any,1}(0)
(::Type{Vector{T}}){T}(m::Integer) = Array{T,1}(Int(m))
(::Type{Vector})(m::Integer) = Array{Any,1}(Int(m))
(::Type{Matrix})() = Array{Any,2}(0, 0)
(::Type{Matrix{T}}){T}(m::Integer, n::Integer) = Matrix{T}(Int(m), Int(n))
(::Type{Matrix})(m::Integer, n::Integer) = Matrix{Any}(Int(m), Int(n))

similar{T}(a::AbstractArray{T}, dims::DimOrInd...) = similar(a, T, to_shape(dims))

BitArray(dims::Integer...) = BitArray(map(Int,dims))

mmap{T<:Array}(::Type{T}, i::Integer...; shared::Bool=true) = mmap(Anonymous(), T, convert(Tuple{Vararg{Int}},i), Int64(0); shared=shared)

rand(dims::Integer...) = rand(convert(Tuple{Vararg{Int}}, dims))

(::Type{SharedArray{T}}){T}(d::Integer...; kwargs...) =
SharedArray(T, d; kwargs...)
(::Type{SharedArray{T}}){T}(m::Integer; kwargs...) =
SharedArray(T, m; kwargs...)
(::Type{SharedArray{T}}){T}(m::Integer, n::Integer; kwargs...) =
SharedArray(T, m, n; kwargs...)
(::Type{SharedArray{T}}){T}(m::Integer, n::Integer, o::Integer; kwargs...) =
SharedArray(T, m, n, o; kwargs...)

It would be great to come up with some kind of consistent policy for 0.6.

@JeffBezanson
Copy link
Member Author

Fair enough; I will drop fixes #17372 from this, and we will have to explore a more comprehensive change from Int to Integer in 0.6. Will just fix the inconsistency for now.

@timholy
Copy link
Member

timholy commented Jul 22, 2016

Or we can go from Integer to Int; I don't really care which we choose. I think the marginal benefit of allowing Integer is pretty small.

@timholy
Copy link
Member

timholy commented Jul 22, 2016

Hmm, though maybe 32bit systems mmapping files > 2GB is an argument to allow Integer.

@JeffBezanson JeffBezanson merged commit 11a34aa into master Jul 23, 2016
@tkelman tkelman deleted the jb/reshapeint branch July 23, 2016 01:39
@tkelman
Copy link
Contributor

tkelman commented Jul 23, 2016

x-ref #16582 as well

mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Before, the definition of `reshape` that returns a `ReshapedArray` accepted
any integer, but the Array->Array definition only accepted `Int`, so passing a
different type of integer strangely resulted in a different type of array.
This behavior was observed as part of JuliaLang#17372.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants