Switch to Symbol keys

[Tokazama]

This PR is motivated primarily by the conversation found here JuliaNeuroscience/NeuroCore.jl#1 (comment). I couldn't resist adding in all of the property methods so there are a bit more changes than the initial commit. However, now there's tab completion for properties.

Here's a summary of changes:

• AbstractDict{String,Any} -> AbstractDict{Symbol,Any}
• getindex(::ImageMeta, ::AbstractString) -> getproperty(::ImageMeta, ::Symbol)
• setindex!(::ImageMeta, X, ::AbstractString) -> setproperty!(::ImageMeta, ::Symbol, X)
• haskey(::ImageMeta, ::AbstractString) -> hasproperty(::ImageMeta, ::Symbol)
• get(::ImageMeta, ::AbstractString, default) -> get(::ImageMeta, ::Symbol, default)

I kept get around because I thought it might be useful to have a method of providing
a default returned value.

[codecov]

Codecov Report

Merging #58 into master will decrease coverage by 3.12%.
The diff coverage is 86.72%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   88.69%   85.56%   -3.13%     
==========================================
  Files           2        3       +1     
  Lines         168      187      +19     
==========================================
+ Hits          149      160      +11     
- Misses         19       27       +8

Impacted Files	Coverage Δ
src/operators.jl	58.33% <0%> (ø)	⬆️
src/deprecations.jl	100% <100%> (ø)
src/ImageMetadata.jl	89.3% <87.03%> (-4.45%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5bd26c...f371dd9. Read the comment docs.

[tlnagy]

I'm okay with this change given the nice syntactical sugar img.property instead of img["property"]. This would definitely require a minor release since it is deprecating.

Thoughts @timholy?

[Tokazama]

I haven't yet implemented it as img.property as I'm trying to avoid doing too much in one PR, but if the general consensus is to just get it all done in one go I'll implement it.

[johnnychen94]

The String-to-Symbol change looks good to me. Can you still keep the previous test here just to make sure they don't error? Probably copy them to a new test/deprecation.jl could be fine.

[Tokazama]

Adding in the code to test the deprecations definitely caught some stuff. It should be working now but I had to get a little creative. For example, accessing properties and data through the dot syntax is technically deprecated so I had to make a unique function getproperty_data and getproperty_properties to add deprecation warnings to.

[johnnychen94]

Perhaps there's better way to do this, but here's what I have in mind:

if isdefined(Base, :hasproperty) # or VERSION >= v"1.2"
    import Base: hasproperty
else
    # make one for compatibility
end

[johnnychen94]

With only some minor changes, I believe it's good to go.

[johnnychen94]

Let's merge #60 first, and then re-trigger the test here to make sure it passes on julia v1.3

When a version is tagged, could you also update the documentation as well?

[timholy]

I am in transit for the holidays and don't have time for more than a quick note, other than to say I support this idea but will want to merge it at a time when I have the ability to help deal with any fallout!

[Tokazama]

I am in transit for the holidays and don't have time for more than a quick note, other than to say I support this idea but will want to merge it at a time when I have the ability to help deal with any fallout!

Sounds good. In the mean time I'll get a PR ready for the documentation.

[johnnychen94]

Great job! All my questions are answered and issues are fixed.

[johnnychen94]

All the changes here look good to me, but it would be great if others can have an independent check here.

I plan to merge this in two days so that @Tokazama can continue his development in NeuroScience

[timholy]

I'd like to get JuliaRegistries/General#7342 merged first (or at least before we tag anything with this), just in case this complexifies the bounding.

[timholy]

Love it! Have you done any benchmarking? I'd just like to ensure we don't have performance regressions.

Since this is a breaking change, I'm not limiting myself to comments specifically on your changes, I'm also noticing legacy problems. If you object, I can help with the cleanup, but this is a convenient place to log such concerns.

Other things to fix at the same time:

• data -> arraydata (see data might be too general a name? ImageAxes.jl#30)

[timholy]

Does anything break if we change this to just plain struct? I ask because it might yield performance improvements for certain operations.

[Tokazama]

With mutable

img = ImageMeta(rand(4,4))

@btime setproperty!(img, :prop1, [1,2,3])

  48.967 ns (1 allocation: 112 bytes)
Dict{Symbol,Any} with 1 entry:
  :prop1 => [1, 2, 3]

@btime getproperty(img, :prop1)
  22.753 ns (0 allocations: 0 bytes)
3-element Array{Int64,1}:
 1
 2
 3

Without

img = ImageMeta(rand(4,4))

julia> @btime setproperty!(img, :prop1, [1,2,3])
  48.609 ns (1 allocation: 112 bytes)
Dict{Symbol,Any} with 1 entry:
  :prop1 => [1, 2, 3]

julia> @btime getproperty(img, :prop1)
  22.099 ns (0 allocations: 0 bytes)
3-element Array{Int64,1}:
 1
 2
 3

[timholy]

Dict accesses are so slow that I very much doubt you'd see any effect there. Array element accesses might be faster with struct (immutability is sometimes a huge advantage for compiler optimizations).

In general I'm not sure I see a reason to allow people to swap in a different data array or different properties dict and call it the same object; the point of the wrapper seems to be to associate this properties dict with this array. But I am willing to be persuaded otherwise.

[Tokazama]

In general I'm not sure I see a reason to allow people to swap in a different data array or different properties dict and call it the same object

I completely agree with this sentiment. I should have clarified what I was demonstrating with the benchmark. It shows a pretty small improvement, but I'm pretty sure that it could result in some nice performance gains when used in combination with some custom properties fields.

[timholy]

Good. Other benchmarks:

julia> struct WrapperI{A,P}
           data::A
           props::P
       end

julia> mutable struct WrapperM{A,P}
           data::A
           props::P
       end

julia> Base.getindex(w::WrapperI, i) = w.data[i]

julia> Base.getindex(w::WrapperM, i) = w.data[i]

julia> data = rand(100); props = Dict(:hi=>"everyone")
Dict{Symbol,String} with 1 entry:
  :hi => "everyone"

julia> wi = WrapperI(data, props);

julia> wm = WrapperM(data, props);

julia> using BenchmarkTools

julia> @btime $wi[3]
  1.481 ns (0 allocations: 0 bytes)
0.4001678030037503

julia> @btime $wm[3]
  1.973 ns (0 allocations: 0 bytes)
0.4001678030037503

So it's a fairly large ratio.

[Tokazama]

This is implemented now. But there's a bit of quirky behavior when trying to set the data property.

ulia> d1 = rand(2,2)
2×2 Array{Float64,2}:
 0.32082   0.533365
 0.719087  0.432655

julia> d2 = rand(2,2)
2×2 Array{Float64,2}:
 0.491014  0.665869
 0.502986  0.535114

julia> img = ImageMeta(d1)
Float64 ImageMeta with:
  data: 2×2 Array{Float64,2}
  properties:

julia> img.data = d2
2×2 Array{Float64,2}:
 0.491014  0.665869
 0.502986  0.535114

julia> img
Float64 ImageMeta with:
  data: 2×2 Array{Float64,2}
  properties:
    data: [0.491014 0.665869; 0.502986 0.535114]

It's not completely clear to me what the best way to handle this is. I could do something like this

function Base.setproperty!(img::ImageMeta, propname::Symbol, val)
    # TODO remove these once deprecations are done
    if propname === :data
        Base.depwarn("setproperty(img, data, val) is deprecated use setindex!(img, :, val) instead", :rawdata)
        setindex!(img, val, img)
    elseif propname === :properties
        Base.depwarn("img.properties is deprecated use properties(img) to extract data img.", :properties)
        for (k,v) in val
            setproperty!(img, k, v)
        end
    else
        properties(img)[propname] = val
    end
    return img
end

[timholy]

Do we have to support setting data? For example, is there a test that fails? What I want to move to is that if you want to replace the data, just create a new ImageMeta wrapper.

[Tokazama]

Do we have to support setting data?

No. I'm just trying to cover all our bases for deprecation warnings.

For example, is there a test that fails? What I want to move to is that if you want to replace the data, just create a new ImageMeta wrapper.

No test fails. Just trying to be thorough here. Because if someone was using img.data = rand(2,2) before this PR they might be upset that it just sets a property named data now. I don't know why anyone would be replacing these fields so if everyone's fine with the way setproperty! works right now then I'll just leave it.

[timholy]

OK. Yes, let's include the functionality and deprecation warning, but also mention creating a new wrapper as an option.

[Tokazama]

setproperty! deprecations is now implemented with suggestions to make new ImageMeta instead.

[timholy]

Bless you! And thanks @johnnychen94 for suggesting it.

[Tokazama]

Something I'm playing around with that this PR permits (but doesn't implement) is performance optimization in the properties field when it's important. I don't have a clean minimal reproducible example, but the general idea is something like this:

using CoordinateTransformations

mutable struct RotationProperties{M<:AbstractDict{Symbol,Any}} <: AbstractDict{Symbol,Any}
   affine_map::AffineMap
   properties::M
end

function Base.getproperty(img::ImageMeta{T,N,<:AbstractArray{T,N},<:RotationProperties}, s::Symbol) where {T,N}
    if s === :affine_map
        return properties(img).affine_map
    else
        return properties(img)[s]
   end
end

I have some ideas to make this more elegant and easy to customize for specific situations still, but the general idea still holds up. Users get a universal interface through getproperty but developers can explicitly define fields that are implicated in performance critical operations. My initial tests show this has a pretty impressive speed up, but I need to get the balance between customizability and intuitive design in place first.

[timholy]

I like it. I've thought a fair bit about orientation issues too; I've noticed that orientation usually requires a bit more explanation than just blatting out an affine transformation. (Is it the forward transformation or the reverse transformation?) I find the encoding format described by ?spacedirections to be substantially clearer, and of course these vectors form the columns of an affine transformation. I have some upcoming applications where the data axes are actually sampled with a skew (i.e., the physical axes are not orthogonal), and in such cases it's particularly important to be clear about exactly what you mean.

[Tokazama]

I've thought a fair bit about orientation issues too

TBH I've spent more time worrying about facilitating flexibility and optimization in accessing and setting properties than orientation itself. I probably have to discuss spatial registration of brains with people almost weekly for quality control purposes. Therefore, my interest in the subject is more based on a constant need to correct inappropriate inferences from data than being an active contributor to registration algorithms. So if you need help with effectively comparing algorithm performance across various applications I have a lot to say, but if you need a mathematical proof I'm no help.

[timholy]

I wasn't really referring to registration (although yes I do work on that), merely "how do you encode orientation unambiguously."

[Tokazama]

I wasn't really referring to registration

Apologies if I got off topic. I mainly brought up registration because it plays a role in how NIfTI encodes orientation. The original scanner orientation is encoded as a quaternion and the newly registered anatomical orientation is encoded as an affine matrix. It's then further complicated with a somewhat poorly defined criteria that the axes all be reorthogonalized and correspond to axial, sagittal, and coronal views. I think each method of encoding has pros and cons.

I'll try to put together something more tractable where this and other properties can be discussed in more detail in the coming days.

[timholy]

This looks good to me. Barring any objections, I plan to merge this tomorrow.

[timholy]

...and it's out as 0.9.0. Thanks for the amazing contribution, @Tokazama!