Rewrite for julia 0.7

[timholy]

The strategy used here for ColorView and ChannelView has been implemented in a much more general way for Base.ReinterpretArray. It makes sense to leverage the Base implementation. Plus, now that reinterpret is no longer restricted to Array, it turns out that we run into certain dispatch problems that stem from changing the dimensionality to handle/subsume color channels. The solution here is to introduce a reinterpretc that is specifically for handling colors. Consequently this is completely breaking.

Not working fully yet, though lots of progress has been made.

[timholy]

Since this is going to be a breaking change, let me be more explicit about what's going on here.

Prior to julia 0.7, it's approximately true to say that the only kind of AbstractArray that one could call reinterpret on was Array. The reason why is that reinterpret was effectively a "pointer cast" operation like in C---it just provided a different view on a contiguous block of memory. But in Julia there's all kinds of cool stuff that can be done with a whole zoo of AbstractArrays that are not Arrays and cannot be converted to one without copying data. In order to support lazy operations on any AbstractArray, this package defined new ColorView and ChannelView AbstractArray types, which conceptually did the same work as reinterpret but worked for any AbstractArray.

Unfortunately these types came with some (a few-fold) performance hit; the reason for the benchmarks as part of the automatic tests in this package was to ensure that this hit didn't become too large, and in particular that some hard work I'd done on optimization when I first wrote this package didn't decay due to later changes. But I also had a second strategy: in some cases, one could safely use reinterpret to perform this operation, and when applicable that didn't come with a performance hit. The lowercase variants, colorview and channelview, used reinterpret when they could (returning another Array) and otherwise created ColorView and ChannelView wrappers.

With JuliaLang/julia#23750, all of these considerations changed. Currently in julia 0.7, reinterpret always returns a ReinterpretArray, a lazy-wrapper that generalizes the strategy I pursued with ColorView and ChannelView, making it possible to call reinterpret on any AbstractArray. While greatly expanding the scope of reinterpret, it simultaneously deprecated a previous capability of reinterpret to control the shape (aka dimensions or size) of the resulting object. (Now you call reshape(reinterpret(T, A), sz) to achieve the same outcome.) IMO this is also a good thing, because it makes Julia's design more orthogonal.

The consequences for this package are quite interesting. It no longer makes sense to have our own ColorView and ChannelView types, instead we should just use reinterpret, or rather reshape(reinterpret(...)) to also split out/eliminate the color channel as a separate axis of the array. Moreover, we can't (or perhaps, shouldn't) define our own specialization of reinterpret that does the appropriate shaping automatically: any reinterpret specialized on, say, AbstractArray{<:Colorant} would have to first call the "plain" reinterpret on the same object, so that leads to a dispatch problem. (We could solve it with invoke, and I'm open to changing this PR to do so, but personally I don't favor this approach.) So instead this PR defines a new function, reinterpretc, which combines reshape and reinterpret appropriately for operations involving arrays of colors. The ColorView and ChannelView types are deleted, but we still retain the colorview and channelview lowercase variants as convenience operations. The way I've implemented it, it's actually even more than "convenience": I've given them some smarts, so that e.g., colorview(eltype(A), channelview(A)) will return A itself rather than creating a wrapper-of-a-wrapper.

Since we no longer have specialized types, it makes much more sense for similar to just create Arrays. This will fix #48. Whew.

Aside from being busy with changes to Julia itself in preparation for 0.7/1.0, the main reason I haven't merged this yet is that unfortunately, the performance hit seems much worse: at least one of the benchmarks is now something like a 20x when formerly it was (IIRC) something like 3x. Moreover, it doesn't seem possible to avoid a performance hit because reinterpret always returns a ReinterpretArray. This was first reported in a different context (JuliaLang/julia#25014) but seems to affect this package pretty severely.

I am hopeful that before or around the time of the 1.0 release, that performance hit will be fixed. It's something I'm a bit concerned about, but I also firmly believe there are much bigger priorities right now. Still, I'll CC @Keno so he's aware of the considerations.

[Keno]

I intend to have the performance fixed by the time 0.7 is released. The performance was optimal at some point on my branch, but something clearly regressed. Just busy with semantic changes at the moment. On Dec 27, 2017 13:18, "Tim Holy" <[email protected]> wrote: Since this is going to be a breaking change, let me be more explicit about what's going on here. Prior to julia 0.7, it's approximately true to say that the only kind of AbstractArray that one could call reinterpret on was Array. The reason why is that reinterpret was effectively a "pointer cast" operation like in C---it just provided a different view on a contiguous block of memory. But in Julia there's all kinds of cool stuff that can be done with a whole zoo of AbstractArrays that are not Arrays and cannot be converted to one without copying data. In order to support lazy operations on any AbstractArray, this package defined new ColorView and ChannelView AbstractArray types, which conceptually did the same work as reinterpret but worked for any AbstractArray. Unfortunately these types came with some (a few-fold) performance hit; the reason for the benchmarks <https://github.com/JuliaImages/ImageCore.jl/blob/master/test/benchmarks.jl> as part of the automatic tests in this package was to ensure that this hit didn't become too large, and in particular that some hard work I'd done on optimization when I first wrote this package didn't decay due to later changes. But I also had a second strategy: in some cases, one *could* safely use reinterpret to perform this operation, and when applicable that *didn't* come with a performance hit. The lowercase variants, colorview and channelview, used reinterpret when they could (returning another Array) and otherwise created ColorView and ChannelView wrappers. With JuliaLang/julia#23750 <JuliaLang/julia#23750>, all of these considerations changed. Currently in julia 0.7, reinterpret *always* returns a ReinterpretArray, a lazy-wrapper that generalizes the strategy I pursued with ColorView and ChannelView, making it possible to call reinterpret on any AbstractArray. While greatly expanding the scope of reinterpret, it simultaneously deprecated a previous capability of reinterpret to control the shape (aka dimensions or size) of the resulting object. (Now you call reshape(reinterpret(T, A), sz) to achieve the same outcome.) IMO this is also a good thing, because it makes Julia's design more orthogonal. The consequences for this package are quite interesting. It no longer makes sense to have our own ColorView and ChannelView types, instead we should just use reinterpret, or rather reshape(reinterpret(...)) to also split out/eliminate the color channel as a separate axis of the array. Moreover, we can't (or perhaps, shouldn't) define our own specialization of reinterpret that does the appropriate shaping automatically: any reinterpret specialized on, say, AbstractArray{<:Colorant} would have to first call the "plain" reinterpret on the same object, so that leads to a dispatch problem. (We could solve it with invoke, and I'm open to changing this PR to do so, but personally I don't favor this approach.) So instead this PR defines a new function, reinterpretc, which combines reshape and reinterpret appropriately for operations involving arrays of colors. The ColorView and ChannelView types are deleted, but we still retain the colorview and channelview lowercase variants as convenience operations. The way I've implemented it, it's actually even more than "convenience": I've given them some smarts, so that e.g., colorview(eltype(A), channelview(A)) will return A itself rather than creating a wrapper-of-a-wrapper. Since we no longer have specialized types, it makes much more sense for similar to just create Arrays. This will fix #48 <#48>. Whew. Aside from being busy with changes to Julia itself in preparation for 0.7/1.0, the main reason I haven't merged this yet is that unfortunately, the performance hit seems much worse: at least one of the benchmarks is now something like a 20x when formerly it was (IIRC) something like 3x. Moreover, it doesn't seem possible to avoid a performance hit because reinterpret always returns a ReinterpretArray. This was first reported in a different context (JuliaLang/julia#25014 <JuliaLang/julia#25014>) but seems to affect this package pretty severely. I am hopeful that before or around the time of the 1.0 release, that performance hit will be fixed. It's something I'm a bit concerned about, but I also firmly believe there are much bigger priorities right now. Still, I'll CC @Keno <https://github.com/keno> so he's aware of the considerations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABO1l9r6jc2TGPb0LiX47Vs7b9E9rbrUks5tEjVsgaJpZM4QiiJN> .

[RalphAS]

I've looked at this with v0.7-beta. I suggest a few fixes:

in src/traits.jl, replace indexes -> indices

in test/runtests.jl, the condition

!Base.root_module_exists(:StatsBase)

may be replaced by

:StatsBase ∉ map(x->Symbol(string(x)),values(Base.loaded_modules))

or

 "StatsBase [2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91]" in Set((string(v) for v = keys(Base.loaded_modules)))

(unless @StefanKarpinski has a better solution.)

size() doesn't work for OffsetArray any more, and because of the revision of reinterpret, the show tests need updating. (Sorry, I don't have anything concrete here.)

With the above patches, (and after suppressing the tests of OffsetArray, show, and one of the FFT subtleties), I find that the benchmark tests pass on v0.7-beta even without further wizardry from Keno. (Other tests also pass.)

[timholy]

Thanks for the tips, @RalphAS! I've updated this branch, it passes except for the benchmarks and a couple of tests I've temporarily commented out. Other things that this waits on:

• ReinterpretArray and IndexStyle JuliaLang/julia#27909
• Fixes needed for ImageCore JuliaGraphics/ColorTypes.jl#108 (just needs tag merged)
• Require 0.7 JuliaArrays/PaddedViews.jl#14 (just needs tag merged)
• Update to Julia 0.7 JuliaArrays/MappedArrays.jl#20 (just needs tag merged)
• tagging of AbstractFFTs and FFTW
• Add OneTo(::AbstractUnitRange) constructors JuliaLang/julia#27931
• setindex! for mappedarray(f, finv, A, B, C...) (and deprecate f_finv tuple)  JuliaArrays/MappedArrays.jl#21
• Support arbitrary axes for ReinterpretArray JuliaLang/julia#27941
• Check bounds on sizeof(eltype)-changing ReinterpretArrays JuliaLang/julia#28073
• Introduce full support for array operations JuliaArrays/OffsetArrays.jl#52
• Remove the size guards JuliaArrays/PaddedViews.jl#15

I will keep thinking about the disabled tests, but I wanted to push this in case it helps anyone else get started on some of the later packages in the Images hierarchy. I suspect that this is the last "hard" case, the rest should be pretty automatic (and help would be appreciated).

[timholy]

OK, I've got the rest working locally. Will wait a bit for comments on the outstanding items above, but hopefully it will happen soon.

[timholy]

Now it's just a waiting game; most likely this will merge tomorrow morning. For anyone using this, note that the julia version is currently marked as ?? given that we are waiting on one more julia PR.

[timholy]

OSX and Windows failures are because the Julia version is too old (0.7.0-beta274 on OSX and 0.7.0-beta.275 or 266 on Windows, whereas I have this marked a minimum of 0.7.0-beta283; specifically JuliaLang/julia#28073 is missing). Given that my time is almost up (I'll be gone for two weeks), I'm going to merge and tag.