Skip to content

Making AxisArrays compatible with new core traits.#33

Draft
Tokazama wants to merge 1 commit intoJuliaImages:masterfrom
Tokazama:master
Draft

Making AxisArrays compatible with new core traits.#33
Tokazama wants to merge 1 commit intoJuliaImages:masterfrom
Tokazama:master

Conversation

@Tokazama
Copy link

The goal here is to make AxisArrays compatible with the new traits added to ImageCore. This adds 3 methods to AxisArrays.


ImageCore.HasDimNames(::Type{A}) where {A<:AxisArray} = HasDimNames{true}()
ImageCore.namedaxes(a::AxisArray) = NamedTuple{axisnames(a)}(axisvalues(a))
Base.names(a::AxisArray) = axisnames(a)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of iffy but it is necessary to really fulfill the intended meaning of HasDimNames.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me why the name of this function needs to be names? (Link to other package, for example?) The reason I ask:

julia> t = (x = 1, y = 2)
(x = 1, y = 2)

julia> typeof(t)
NamedTuple{(:x, :y),Tuple{Int64,Int64}}

julia> names(t)
ERROR: MethodError: no method matching names(::NamedTuple{(:x, :y),Tuple{Int64,Int64}})
Closest candidates are:
  names(::Module; all, imported) at reflection.jl:98
Stacktrace:
 [1] top-level scope at REPL[3]:1

struct, for example, uses fieldnames rather than just names. axisnames seems like a really good name for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what NamedDims uses to access the axis names https://github.com/invenia/NamedDims.jl/blob/master/src/wrapper_array.jl#L77.

But hadn't even considered using a different name. axisnames would certainly be more descriptive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what happens in invenia/NamedDims.jl#22

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #33 into master will decrease coverage by 0.33%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   81.95%   81.61%   -0.34%     
==========================================
  Files           1        1              
  Lines         133      136       +3     
==========================================
+ Hits          109      111       +2     
- Misses         24       25       +1
Impacted Files Coverage Δ
src/ImageAxes.jl 81.61% <66.66%> (-0.34%) ⬇️

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 e6f7a61...bcaf598. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants