Closed Evizero closed 6 years ago
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.
What about renaming axes to prevent the name clash with Julia base? @timholy was in favor here https://github.com/JuliaImages/ImageAxes.jl/issues/25#issuecomment-354538168 and I kind of agree. Unfortunately I don't have a better idea right now.
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.
I am fine with that as well, just wanted to bring this to attention.
@mbauman Did you get further on this past the commits you've pushed?
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
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!
@mbauman: Why should we export axes
? If this is not done, there would not be the conflict dropping also the Base axes
julia> module MyModule
axes() = println("axes called")
end
julia> MyModule.axes()
axes called
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.
I see, fine with that as a stopgap solution!
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
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!
Not even the core.jl tests pass, but its fixes some of the more obvious warnings at least