[WIP] fixes for julia 0.7 #37

fredrikekre · 2017-12-05T10:36:52Z

Split out from #34: This PR aims to just get things working on julia master, and then I will rebase #34 on top of this.

So after investigating a bit further regarding the similiar methods discussed in #34 there seem to be two problems:

With the new broadcasting interface, broadcast_similar used the fallback method. I fixed this by hooking into the broadcast machinery (excellent interface btw Tim 🎉)

The other problems show up in the tests I commented out for now. For instance:

julia> A = OffsetArray(rand(4,4), (-3,5));

julia> unique(A, 1)
ERROR: MethodError: no method matching Array{Int64,N} where N(::Uninitialized, ::Tuple{UnitRange{Int64}})
Stacktrace:
 [1] similar(::Type{Array{Int64,N} where N}, ::Tuple{UnitRange{Int64}}) at ./array.jl:254
 [2] similar(::Type, ::UnitRange{Int64}) at ./abstractarray.jl:567
 [3] @generated body at ./multidimensional.jl:1583 [inlined]
 [4] unique(::OffsetArray{Float64,2,Array{Float64,2}}, ::Int64) at ./multidimensional.jl:1571
 [5] top-level scope

which is because unique calls the similar(::Type{BitArray}, inds) method here: https://github.com/JuliaLang/julia/blob/c697415e5d665e2085846dc3a3313cff296ef98f/base/multidimensional.jl#L1606 which forces us to hijack that method.

Similarly, for sortcols / sortrows we end up here: https://github.com/JuliaLang/julia/blob/c697415e5d665e2085846dc3a3313cff296ef98f/base/sort.jl#L964 which forces us to hijack that method. sortcols / sortrows can be fixed, I think with the following patch to base:

diff --git a/base/sort.jl b/base/sort.jl
index 2c7a0ac..ca7bbf6 100644
--- a/base/sort.jl
+++ b/base/sort.jl
@@ -922,7 +922,7 @@ julia> sortrows([7 3 5; -1 6 4; 9 -2 8], rev=true)
 function sortrows(A::AbstractMatrix; kws...)
     inds = indices(A,1)
     T = slicetypeof(A, inds, :)
-    rows = similar(Vector{T}, indices(A, 1))
+    rows = similar(A, T, indices(A, 1))
     for i in inds
         rows[i] = view(A, i, :)
     end
@@ -961,7 +961,7 @@ julia> sortcols([7 3 5; 6 -1 -4; 9 -2 8], rev=true)
 function sortcols(A::AbstractMatrix; kws...)
     inds = indices(A,2)
     T = slicetypeof(A, :, inds)
-    cols = similar(Vector{T}, indices(A, 2))
+    cols = similar(A, T, indices(A, 2))
     for i in inds
         cols[i] = view(A, :, i)
     end

timholy

👍

timholy · 2017-12-05T22:53:07Z

src/OffsetArrays.jl

+# Broadcasting interface
+@static if VERSION > v"0.7.0-DEV.2638" # https://github.com/JuliaLang/julia/pull/23939
+    struct OffsetArrayStyle <: Broadcast.BroadcastStyle end
+    Broadcast.BroadcastStyle(::Type{<:OffsetArray}) = OffsetArrayStyle()


I think you can save several lines with

Broadcast.BroadcastStyle(::Type{<:OffsetArray}) = Broadcast.ArrayStyle{OffsetArray}()

You shouldn't need broadcast_indices or define the rule with respect to Scalar. More importantly, it should also successfully take precedence over, e.g., Array.

timholy · 2017-12-05T22:54:44Z

test/runtests.jl

@@ -312,12 +312,12 @@ seek(io, 0)
 amin, amax = extrema(parent(A))
 @test clamp.(A, (amax+amin)/2, amax) == OffsetArray(clamp.(parent(A), (amax+amin)/2, amax), indices(A))

-@test unique(A, 1) == parent(A)
-@test unique(A, 2) == parent(A)
+# @test unique(A, 1) == parent(A)


Why do these need to be commented out?

Oh, sorry, I see you explained.

I'm less worried about the hijacking than you. To me similar(BitArray, inds) means "give me something that acts like a BitArray but has the following inds." If that's not a BitArray (generally, it will turn out to be a wrapped BitArray), that's fine.

timholy · 2017-12-05T22:54:54Z

test/runtests.jl

 v = OffsetArray(rand(8), (-2,))
 @test sort(v) == OffsetArray(sort(parent(v)), v.offsets)
-@test sortrows(A) == OffsetArray(sortrows(parent(A)), A.offsets)
-@test sortcols(A) == OffsetArray(sortcols(parent(A)), A.offsets)
+# @test sortrows(A) == OffsetArray(sortrows(parent(A)), A.offsets)


fredrikekre · 2017-12-06T11:04:50Z

Alright, this just needs JuliaLang/Compat.jl#427 and JuliaLang/Compat.jl#429 now, then a new Compat version.

timholy · 2017-12-06T11:21:44Z

src/OffsetArrays.jl

+    OffsetArray(T(uninitialized, map(length, shape)), map(indexoffset, shape))
+
+# Broadcasting interface
+@static if VERSION > v"0.7.0-DEV.2638" # https://github.com/JuliaLang/julia/pull/23939


Do we need this anymore or is it handled by similar?

No 😞 But it was fun to try the new interface 😄

timholy · 2017-12-06T11:23:28Z

src/OffsetArrays.jl

+Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:Array} =
+    OffsetArray(T(uninitialized, map(length, shape)), map(indexoffset, shape))
+Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:BitArray} =
+    OffsetArray(T(uninitialized, map(length, shape)), map(indexoffset, shape))


Do we need all three of these specializations or would

Base.similar(::Type{T}, shape::Tuple{UnitRange,Vararg{UnitRange}}) where {T<:AbstractArray} = OffsetArray(T(map(length, shape)), map(indexoffset, shape))

be good enough? (OffsetArray being the defacto "owner" of UnitRange indices.)

Yes, the are needed for ambiguity resolution with https://github.com/JuliaLang/julia/blob/7d7d81af41448aeaafb2dadedf54e831616c25cf/base/array.jl#L254 and https://github.com/JuliaLang/julia/blob/7d7d81af41448aeaafb2dadedf54e831616c25cf/base/bitarray.jl#L355. And we also need a separate for OffsetArray, which does not use the new uninitialized singleton for construction. I plan to implement that in a separate PR.

timholy · 2017-12-12T08:57:21Z

This seems ready to go except for the 0.6 breakage.

fredrikekre · 2017-12-12T09:00:04Z

Yes, for 0.6 this needs a new Compat tag for the IOContext change (JuliaLang/Compat.jl#427), was waiting for JuliaLang/Compat.jl#428 before tagging.

- indices -> axes - ind2sub -> CartesianIndices - CartesianRange -> CartesianIndices - copy! -> copyto!

fredrikekre · 2018-01-12T14:11:50Z

Looks like tests passes on both 0.6 and 0.7 now :)

timholy · 2018-01-13T11:28:39Z

Wonderful, many thanks for seeing this through.

fredrikekre mentioned this pull request Dec 5, 2017

change similar calls in sortcols and sortrows JuliaLang/julia#24929

Merged

timholy reviewed Dec 5, 2017

View reviewed changes

fredrikekre force-pushed the fe/fixes-for-julia-0.7 branch 2 times, most recently from f6b40d3 to ca511d6 Compare December 6, 2017 10:43

timholy reviewed Dec 6, 2017

View reviewed changes

fredrikekre force-pushed the fe/fixes-for-julia-0.7 branch from ca511d6 to 8c8f4f3 Compare December 6, 2017 11:52

timholy closed this Dec 12, 2017

timholy reopened this Dec 12, 2017

fredrikekre force-pushed the fe/fixes-for-julia-0.7 branch from 8c8f4f3 to 9d0e8c3 Compare December 21, 2017 12:18

fredrikekre closed this Dec 21, 2017

fredrikekre reopened this Dec 21, 2017

fredrikekre force-pushed the fe/fixes-for-julia-0.7 branch 3 times, most recently from b06b453 to 1988cf2 Compare January 9, 2018 20:48

- deprecation fixes for julia master

ce854a7

- indices -> axes - ind2sub -> CartesianIndices - CartesianRange -> CartesianIndices - copy! -> copyto!

fredrikekre force-pushed the fe/fixes-for-julia-0.7 branch from 1988cf2 to ce854a7 Compare January 9, 2018 20:53

fredrikekre closed this Jan 12, 2018

fredrikekre reopened this Jan 12, 2018

timholy merged commit e0ec9ed into JuliaArrays:master Jan 13, 2018

fredrikekre deleted the fe/fixes-for-julia-0.7 branch January 13, 2018 12:35

This was referenced Jan 13, 2018

deprecate constructors with type as first argument #34

Merged

Use uninitialized in constructor #39

Closed

Rename indices to axes #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fixes for julia 0.7 #37

[WIP] fixes for julia 0.7 #37

fredrikekre commented Dec 5, 2017 •

edited

Loading

timholy left a comment

timholy Dec 5, 2017

timholy Dec 5, 2017

timholy Dec 5, 2017

timholy Dec 5, 2017

fredrikekre commented Dec 6, 2017

timholy Dec 6, 2017

fredrikekre Dec 6, 2017

timholy Dec 6, 2017

fredrikekre Dec 6, 2017

timholy commented Dec 12, 2017

fredrikekre commented Dec 12, 2017 •

edited

Loading

fredrikekre commented Jan 12, 2018

timholy commented Jan 13, 2018

[WIP] fixes for julia 0.7 #37

[WIP] fixes for julia 0.7 #37

Conversation

fredrikekre commented Dec 5, 2017 • edited Loading

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Dec 12, 2017

fredrikekre commented Dec 12, 2017 • edited Loading

fredrikekre commented Jan 12, 2018

timholy commented Jan 13, 2018

fredrikekre commented Dec 5, 2017 •

edited

Loading

fredrikekre commented Dec 12, 2017 •

edited

Loading