-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide trait based opt in for Images API (take 2) #86
Conversation
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
=========================================
+ Coverage 66.66% 66.9% +0.23%
=========================================
Files 9 9
Lines 414 426 +12
=========================================
+ Hits 276 285 +9
- Misses 138 141 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
==========================================
+ Coverage 66.66% 67.45% +0.78%
==========================================
Files 9 9
Lines 414 424 +10
==========================================
+ Hits 276 286 +10
Misses 138 138
Continue to review full report at Codecov.
|
I think Julia inlined my code so it's not getting caught by Codecov (the lines not covered are the default axis names which are indirectly tested by different array sizes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea since it's absolutely an add-on feature. Most of the existing codes won't be affected unless the developers decide to take advantages of axis information.
src/traits.jl
Outdated
ntuple(i -> default_name(i), N)::NTuple{N,Symbol} | ||
end | ||
|
||
@inline function default_name(i::Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like :dim_1
, :dim_2
, :dim_3
since it provides nothing useful by default.
Providing meaningful axes for it might cause some unexpected behaviors, e.g.,:
img = rand(RGB, 2, 2)
namedaxes(channelview(img)) # it should be (:channel, :row, :col)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default names were pulled from what AxisArrays
uses. I was trying to be conservative with what I changed, but I agree that his makes more sense as a default.
`default_names` is now a simple generated function, because the symbol names weren't being interpolated at compile time. Hopefully dispatching on `Vall{N}` instead of the full array will limit the number of functions that have to be generated.
LGTM, @timholy how do you think? |
bump |
Thanks! |
This is following up on JuliaImages/Images.jl#815.
The idea is to provide a bear bones interface for acting within the
Images.jl
ecosystem. It also aims to disrupt everything else as little as possible when adopted by other packages in the JuliaImages organization.