Towards 0.7 support #143

Evizero · 2018-08-05T14:31:39Z

Not even the core.jl tests pass, but its fixes some of the more obvious warnings at least

Evizero · 2018-08-05T14:32:04Z

src/categoricalvector.jl

@@ -43,17 +43,13 @@ struct CategoricalVector{T, A<:AbstractVector{T}} <: AbstractVector{T}
    data::A
 end

-function CategoricalVector(data::AbstractVector{T}) where T
-    CategoricalVector{T, typeof(data)}(data)
-end


the default constructor takes care of this. so this caused a redefinition warning.

Evizero · 2018-08-05T14:36:41Z

test/core.jl

@@ -1,3 +1,4 @@
+# FIXME: type stability broken. The following should NOT error
 A = @inferred(AxisArray(reshape(1:24, 2,3,4), .1:.1:.2, .1:.1:.3, .1:.1:.4))


The type instability comes from default_axes which i tried to fix, but couldn't

Its quite strange that the type instability usually happens for the last tuple element

julia> @code_warntype AxisArrays.default_axes(reshape(1:24, 2,3,4), (.1:.1:.2, .1:.1:.3, .1:.1:.4)) Body::Tuple{Axis,Axis,Axis} 172 1 ─ %1 = (getfield)(args, 2)::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} │╻╷ _default_axes │ %2 = (getfield)(args, 3)::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} ││┃ tail │ %3 = (Base.getfield)(args, 1, true)::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} ││╻ getindex │ %4 = %new(Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}, %3)::Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}} │ %5 = (Core.tuple)(%2)::Tuple{StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}} │││╻ tail │ %6 = %new(Axis{:col,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}, %1)::Axis{:col,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}} │ %7 = (Core.tuple)(%4, %6)::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}} │ %8 = invoke AxisArrays._default_axes(_2::Base.ReshapedArray{Int64,3,UnitRange{Int64},Tuple{}}, %5::Tuple{StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}, %7::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}})::Tuple{Axis,Axis,Axis} └── return %8

Evizero · 2018-08-05T14:40:35Z

test/core.jl

@@ -106,6 +107,7 @@ A = @inferred(AxisArray(1:3, .1:.1:.3))
 @test axisnames(A) == (:row,)
 @inferred(axisnames(A))
 @test axisvalues(A) == (.1:.1:.3,)
+# FIXME: reintroduce inferred
 A = @inferred(AxisArray(reshape(1:16, 2,2,2,2), .5:.5:1))


same as above. type stability breaks with last tuple element for defaul_axes

julia> @code_warntype AxisArrays.default_axes(reshape(1:16, 2,2,2,2), (.5:.5:1,)) Body::NTuple{4,Axis} 172 1 ─ %1 = (Base.getfield)(args, 1, true)::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} │╻╷ _default_axes │ %2 = %new(Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}, %1)::Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}} │ %3 = (Base.getfield)(A, :dims)::NTuple{4,Int64} │││╻╷╷╷ axes │ %4 = (Base.getfield)(%3, 1, true)::Int64 ││││╻ axes │ %22 = (getfield)(%18, 2)::Int64 │││││││╻ tail [...] │ (Base.ifelse)(%30, 0, %24) ││││││││││┃ max │ %32 = %new(Axis{:page,Base.OneTo{Int64}}, %29)::Axis{:page,Base.OneTo{Int64}} │││││╻ Type │ %33 = (Core.tuple)(%2, %17, %32)::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Base.OneTo{Int64}},Axis{:page,Base.OneTo{Int64}}} │ %34 = invoke AxisArrays._default_axes(_2::Base.ReshapedArray{Int64,4,UnitRange{Int64},Tuple{}}, ()::Tuple{}, %33::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Base.OneTo{Int64}},Axis{:page,Base.OneTo{Int64}}})::NTuple{4,Axis} └── return %34

it works if you only reshape to only 3 dimensions

julia> @code_warntype AxisArrays.default_axes(reshape(1:16, 2,2,4), (.5:.5:1,)) Body::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Base.OneTo{Int64}},Axis{:page,Base.OneTo{Int64}}} 172 1 ─ %1 = (Base.getfield)(args, 1, true)::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}} │╻╷ _default_axes │ %2 = %new(Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}, %1)::Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}} │ %3 = (Base.getfield)(A, :dims)::Tuple{Int64,Int64,Int64} │││╻╷╷╷ axes │ %4 = (Base.getfield)(%3, 1, true)::Int64 ││││╻ axes │ %5 = (Base.slt_int)(%4, 0)::Bool │││││╻╷╷ map [...] │ %25 = %new(Base.OneTo{Int64}, %24)::Base.OneTo{Int64} │││││││││ │ %26 = %new(Axis{:page,Base.OneTo{Int64}}, %25)::Axis{:page,Base.OneTo{Int64}} │││││╻ Type │ %27 = (Core.tuple)(%2, %14, %26)::Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Base.OneTo{Int64}},Axis{:page,Base.OneTo{Int64}}} └── return %27

mbauman · 2018-08-07T12:52:00Z

Thank you so much! I had started in much the same direction on the plane, but I think we can work around this PR here. I'll start pushing commits here.

tknopp · 2018-08-08T23:10:44Z

What about renaming axes to prevent the name clash with Julia base? @timholy was in favor here JuliaImages/ImageAxes.jl#25 (comment) and I kind of agree. Unfortunately I don't have a better idea right now.

mbauman · 2018-08-08T23:32:51Z

My current thought is to get a patched-up AxisArrays out the door with axes defined, and then later refactor the name and/or package.

There's not really a good transition option for AA's dependencies here, at least as far as I've been able to see. So at least continuing to export the name is an obvious fix. And then once the uses are qualified, it's a much easier rename.

the squeeze deprecation will need some more work to be type-stable using a varargs kwarg

tknopp · 2018-08-09T00:08:21Z

I am fine with that as well, just wanted to bring this to attention.

iamed2 · 2018-08-11T13:26:51Z

@mbauman Did you get further on this past the commits you've pushed?

…dices

tknopp · 2018-08-16T08:16:25Z

.travis.yml

@@ -3,19 +3,30 @@ os:
  - linux
  - osx
 julia:
-  - 0.6
+  - 0.7
+  - 1.0


@Evizero: If you do this, you also need to bump REQUIRE to 0.7. I recommend that since a lot of packages drop 0.6 support on master. (@mbauman for attention)

Evizero · 2018-08-16T08:19:32Z

note I am just doing some small changes to help along. like updating travis and removing a couple of depwarns around sum.

I'd like to help more but I feel like chances are I'd end up working against you

mbauman · 2018-08-16T14:09:56Z

That's awesome, thanks! I'm trying to push my work as soon as I achieve a small bit of progress, so don't worry too much about conflicts. I'm back at this now, trying to wrap it up today!

… AxisArray constructor. Simplify make_axes_match.

* More axes qualifiers * Fixup an earlier findall replacement * Simplify reaxis and explicitly implement linear indexing there

tknopp · 2018-08-16T23:00:51Z

@mbauman: Why should we export axes? If this is not done, there would not be the conflict dropping also the Base axes

tknopp · 2018-08-16T23:05:30Z

julia> module MyModule
         axes() = println("axes called")
       end
         
julia> MyModule.axes()
axes called

mbauman · 2018-08-17T04:00:40Z

Yes, we could of course do that. The thought behind the error is that it should make it easier for dependencies to identify the need to change their usages of axes, precisely at the point at which it is called. It's not as graceful as a deprecation mechanism, but it's an error that will serve as the same. The section I added in the README is designed to serve as google-fodder for that error and help folks upgrade.

Once dependencies are upgraded, we can then change the name or unexport it.

I still favor this path.

tknopp · 2018-08-17T06:17:25Z

I see, fine with that as a stopgap solution!

RalphAS · 2018-08-17T11:50:27Z

I encountered a couple of surviving deprecations while trying this out:

┌ Warning: using `A[I...] = x` to implicitly broadcast `x` across many locations is deprecated. Use `A[I...] .= x` instead.
│   caller = setindex! at indexing.jl:108 [inlined]
└ @ Core ~/.julia/dev/AxisArrays/src/indexing.jl:108

┌ Warning: `String(io::GenericIOBuffer)` is deprecated, use `String(take!(copy(io)))` instead.
│   caller = summary(::AxisArray{Gray{Normed{UInt8,8}},2,Array{Gray{Normed{UInt8,8}},2},Tuple{Axis{:y,Base.OneTo{Int64}},Axis{:x,Base.OneTo{Int64}}}}) at core.jl:493
└ @ AxisArrays ~/.julia/dev/AxisArrays/src/core.jl:493

mbauman · 2018-08-17T16:36:43Z

That's great, @RalphAS, thanks! I've fixed the second deprecation you found.

The first is an example where we're simply deferring to (and mirroring) Base's implementation. So it behaves as we want on 1.0, and it does throw a deprecation warning on 0.7 (albeit with the incorrect caller information). I gave a quick shot at deprecating it properly but failed... it's a complicated one. I'm inclined to just merge and tag once tests pass here so we can start upgrading dependencies!

work towards 0.7 support

10cf55f

Evizero commented Aug 5, 2018

View reviewed changes

Use Val() syntax in tests

216c0d9

mbauman added 2 commits August 8, 2018 19:48

Type stable constructors

2376678

lastindex, range, and squeeze deprecations...

2e0b174

the squeeze deprecation will need some more work to be type-stable using a varargs kwarg

mbauman and others added 6 commits August 14, 2018 10:52

Core tests pass!

c75b67e

fixup

51da9a5

Try using a tuple of AxisArrays as "axes"-ish objects from reduced_in…

6c7f562

…dices

fixup

40fe310

Minor ntuple and axes/indices fixes

5f458db

update travis yml

71948c5

tknopp reviewed Aug 16, 2018

View reviewed changes

Evizero added 2 commits August 16, 2018 10:20

drop 0.6 support

f3df570

fix more deprecation warnings

548854d

mbauman added 9 commits August 16, 2018 09:28

More qualifications of axes

a4a0453

Replace _nextaxistype with _default_axis

4f34739

Make default_axes support different numbers of axes, move errors into…

14d28f8

… AxisArray constructor. Simplify make_axes_match.

Indexing fixes:

42fe1a5

* More axes qualifiers * Fixup an earlier findall replacement * Simplify reaxis and explicitly implement linear indexing there

fixup

6723577

Indexing tests pass!

d735145

Fix SortedVector tests (random seeding has changed)

65e82a7

Same deal with categorical vectors

cad080a

find->findall, axes qualification

34accd9

mbauman added 5 commits August 16, 2018 15:07

Combine and join fixes

cf3409f

Disable problematic inference failures

4c83704

final fixes for v"1.0"

8b24170

Try deploying with 1.0; add Unitful for the example

ec8d2de

Add a notice regarding the axes function to the README

b9a1d1d

mbauman added 2 commits August 17, 2018 10:12

Test and fix String(::IOBuffer) deprecation

e7a9f46

Re-enable ambiguity tests on 1.0

8afaac2

Re-enable disabled tests - just don't test at-inferred

343d9ee

mbauman merged commit 8cb003b into JuliaArrays:master Aug 17, 2018

mbauman mentioned this pull request Aug 17, 2018

Update to Julia v0.7 #141

Closed

Evizero deleted the update07 branch August 18, 2018 06:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards 0.7 support #143

Towards 0.7 support #143

Evizero commented Aug 5, 2018

Evizero Aug 5, 2018

Evizero Aug 5, 2018

Evizero Aug 5, 2018

mbauman commented Aug 7, 2018

tknopp commented Aug 8, 2018

mbauman commented Aug 8, 2018

tknopp commented Aug 9, 2018

iamed2 commented Aug 11, 2018

tknopp Aug 16, 2018

Evizero Aug 16, 2018

Evizero commented Aug 16, 2018

mbauman commented Aug 16, 2018

tknopp commented Aug 16, 2018

tknopp commented Aug 16, 2018

mbauman commented Aug 17, 2018

tknopp commented Aug 17, 2018

RalphAS commented Aug 17, 2018

mbauman commented Aug 17, 2018

		@@ -1,3 +1,4 @@
		# FIXME: type stability broken. The following should NOT error
		A = @inferred(AxisArray(reshape(1:24, 2,3,4), .1:.1:.2, .1:.1:.3, .1:.1:.4))

@@ @@ -3,19 +3,30 @@ os: @@
                 - linux
                 - osx
               julia:
-                - 0.6
+                - 0.7
+                - 1.0

Towards 0.7 support #143

Towards 0.7 support #143

Conversation

Evizero commented Aug 5, 2018

Evizero Aug 5, 2018

Choose a reason for hiding this comment

Evizero Aug 5, 2018

Choose a reason for hiding this comment

Evizero Aug 5, 2018

Choose a reason for hiding this comment

mbauman commented Aug 7, 2018

tknopp commented Aug 8, 2018

mbauman commented Aug 8, 2018

tknopp commented Aug 9, 2018

iamed2 commented Aug 11, 2018

tknopp Aug 16, 2018

Choose a reason for hiding this comment

Evizero Aug 16, 2018

Choose a reason for hiding this comment

Evizero commented Aug 16, 2018

mbauman commented Aug 16, 2018

tknopp commented Aug 16, 2018

tknopp commented Aug 16, 2018

mbauman commented Aug 17, 2018

tknopp commented Aug 17, 2018

RalphAS commented Aug 17, 2018

mbauman commented Aug 17, 2018