Support AxisArrays and suppress warnings for the old Images syntax#143
Support AxisArrays and suppress warnings for the old Images syntax#143SimonDanisch merged 2 commits intomasterfrom
Conversation
| end | ||
| end | ||
|
|
||
| function _default{T}(img::HasAxesArray{T,2}, s::Style, data::Dict) |
There was a problem hiding this comment.
This is intended to allow even 2d images to use pixel spacing that is not 1:1. This will be important for accurate rendering of slices in the upcoming "volumes.jl" demo, where the pixelspacing in 3d is (1,1,5) and where two of the 2d slices look wrong without fixing the geometry. However, this is not actually working to solve that problem. Any tips?
There was a problem hiding this comment.
Let me fix this! I will push it with comments to this PR, if that's okay!
[edit]
This is how I'd do this:
https://github.com/JuliaGL/GLVisualize.jl/blob/sd/thaxis/src/visualize/image_like.jl#L157
But the whole code needs some serious refactoring! ;)
There was a problem hiding this comment.
If you're tackling it, I can wait. By all means feel free to push with this branch or run with it in some other direction.
| get!(data, :spatialorder, join(props["spatialorder"], "")) | ||
| # If the user is using the new Images, ImageAxes will be loaded | ||
| if isdefined(Images, :ImageAxes) | ||
| unwrap(img::Images.ImageMeta) = unwrap(data(img)) |
There was a problem hiding this comment.
Is there a convert(Array, AxisArray)? Than we could just convert all AbstractArrays to an Array, solving your view etc problem ;)
Then we'd just need to add AxisArray to the great Array Union... Or actually, I should simply use AbstractArray instead of that and let convert do the rest... Let me look into this :)
There was a problem hiding this comment.
Yes, any AbstractArray can be converted to an Array. The one thing that's a bit ambiguous is whether convert (which is a generic function) should copy or share data: over time, julia has tried to become more consistent about this kind of thing. (For example, I did a lot of work during the 0.5 cycle to make reshape always return a view.) It doesn't seem that this is specified, so I've filed an issue, see JuliaLang/julia#20826.
If it's decided that convert needs to have predictable copy/view behavior, then I imagine you might be interested in a package-specific zero-copy alternative? (Whenever it's possible to do, of course.)
There was a problem hiding this comment.
Oh yeah, I definitely wouldn't like copying, since it will already get copied when transferred to the GPU ;)
Maybe, I should not convert them at all, but do this inside GLAbstraction when copying to the device.
I have more information there, e.g. for buffers, I could even copy from views, since I can get a pointer directly to the GPU memory ;)
There was a problem hiding this comment.
I do think that GLAbstraction is the right place to make this kind of decision, and then to loosen the types on any higher-level functions. As a bonus, any extra information that gets "carried around" (unusual indices, pixel spacing, etc) by an AbstractArray type will be available until the very last possible moment, when you need to transfer a block of memory to the GPU.
There was a problem hiding this comment.
Looks like it's OK for convert to do the minimal thing.
There was a problem hiding this comment.
Okay let me see if I can materialize this in code... One problem is on 0.5, that Signal{AbstractArray{T, N}} doesn't dispatch... Let's see what I can do about it.
There was a problem hiding this comment.
You can do
foo{A<:AbstractArray}(x::Signal{A}) = ...Or do you mean the lack of triangular dispatch? If you have to, one strategy is to create two arguments from a single argument:
foo{C<:Colorant}(A::AbstractArray{C}) = _foo(A, eltype(C))
_foo{C<:Colorant,T<:FixedPoint}(A::AbstractArray{C}, ::Type{T}) = ...A somewhat easier way is to use a typealias:
@compat const FixedColorant{T<:FixedPoint,N} = Colorant{T,N}
@compat const AbstractColorantFixedArray{C<:FixedColorant,N} = AbstractArray{C,N}Though in general I try to avoid dispatching on element type when possible (but sometimes you simply can't avoid it).
| # We could do this as a @traitfn, except that those don't | ||
| # currently mix well with non-trait specialization. | ||
| if timeaxis(img) != nothing | ||
| data[:spatialorder] = "yx" |
There was a problem hiding this comment.
We should just have one function to extract properties like this from an AbstractArray. E.g.:
pixelspacing(A::AbstractArray) = size(A) # fallback
pixelspacing(::AxisArray) = #do your thingThen we can reduce code duplication ;)
There was a problem hiding this comment.
If this function is defined somewhere in Images/AxisArrays, we could even get rid of any reliance on AxisArrays!
There was a problem hiding this comment.
It is defined that way in ImageCore. But timeaxis relies on ImageAxes, which is @reexported by Images, and it relies on AxisArrays. So this is not an "extra" dependency.
There was a problem hiding this comment.
I meant conceptually ;) So that when you read GLVisualize code, you don't get the noise of dealing with all these different array types!
There was a problem hiding this comment.
True. Although I grew to hate data in the old Images, it seemed so random when you used it and when you didn't. That might have been more of a problem of other functions/packages, though.
I suppose we could add a fallback timeaxis for AbstractArrays. But what should it return for an Array{T,3}? I'm inclined towards nothing, which is what it currently returns for any AxisArray that lacks an axis named :time.
| end | ||
| end | ||
|
|
||
| function _default{T}(img::HasAxesArray{T,2}, s::Style, data::Dict) |
There was a problem hiding this comment.
Let me fix this! I will push it with comments to this PR, if that's okay!
[edit]
This is how I'd do this:
https://github.com/JuliaGL/GLVisualize.jl/blob/sd/thaxis/src/visualize/image_like.jl#L157
But the whole code needs some serious refactoring! ;)
|
closed by: #150 |
|
I decided to merge a first version to get things started, and have a clean version when I get to refactoring GLVisualize ;) |
2 participants
This is work motivated by the "volumes.jl" example in GLPlots. Now that VideoIO has migrated to the new Images, I don't really think there's any reason to keep the old one around, but I left it in place just in case. It's a slightly worse experience because of the lack of docstrings and incorrect line numbers (due to the use of
include_string), but I think that's a good thing 😄.The most important step was to infer information about the nature of the image. One of the advantages of the new framework is that any
AbstractArray{T,3}is genuinely 3-dimensional: you can no longer use a dimension to encode color. We could also dispatch separately on images-with-a-time-axis and any other kind of 3d image, but for now this uses branching due to limitations in SimpleTraits.