Skip to content
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

maxabsfinite is breaking #14

Closed
johnnychen94 opened this issue Aug 28, 2021 · 2 comments · Fixed by #17
Closed

maxabsfinite is breaking #14

johnnychen94 opened this issue Aug 28, 2021 · 2 comments · Fixed by #17
Assignees
Labels
bug Something isn't working

Comments

@johnnychen94
Copy link
Member

# in Images
julia> maxabsfinite(rand(RGB, 4, 4))
2.219370615838762

# in ImageBase
julia> maxabsfinite(rand(RGB, 4, 4))
RGB{Float64}(0.9807213248971391,0.9620842090794137,0.9778493728627977)

I also notice that kwargs are not tested.

julia> maxfinite(rand(RGB, 4, 4); dims=1)
ERROR: MethodError: no method matching reducedim_init(::ImageBase.Map12{typeof(isfinite), typeof(identity), typeof(typemin)}, ::typeof(ImageBase.maxc), ::Matrix{RGB{Float64}}, ::Int64)
Closest candidates are:
  reducedim_init(::Any, ::typeof(min), ::AbstractArray, ::Any) at reducedim.jl:129
  reducedim_init(::Any, ::typeof(max), ::AbstractArray, ::Any) at reducedim.jl:129
  reducedim_init(::Any, ::typeof(&), ::Union{Base.AbstractBroadcasted, AbstractArray}, ::Any) at reducedim.jl:159
  ...
Stacktrace:
 [1] _mapreduce_dim(f::ImageBase.Map12{typeof(isfinite), typeof(identity), typeof(typemin)}, op::Function, #unused#::Base._InitialValue, A::Matrix{RGB{Float64}}, dims::Int64)
   @ Base ./reducedim.jl:324
 [2] #mapreduce#672
   @ ./reducedim.jl:310 [inlined]
 [3] maxfinite(A::Matrix{RGB{Float64}}; kwargs::Base.Iterators.Pairs{Symbol, Int64, Tuple{Symbol}, NamedTuple{(:dims,), Tuple{Int64}}})
   @ ImageBase ~/Documents/Julia/ImageBase.jl/src/statistics.jl:34
 [4] top-level scope
   @ REPL[7]:1

Looks like we need more comprehensive tests and compatibility tests for these functions.

@johnnychen94 johnnychen94 added the bug Something isn't working label Aug 28, 2021
@timholy
Copy link
Member

timholy commented Aug 29, 2021

Oh that's interesting. I noticed the tests looked a little thin, but I had no idea that I'd changed the implementation.

Do you think we should keep the original scalar-return, or is channelwise better? We have the chance to make a breaking change, of course, and I don't think it's heavily used.

@johnnychen94
Copy link
Member Author

Without considering the compatibility with Images.jl, which I've fixed in JuliaImages/Images.jl@bad872f, I prefer channelwise output as it gives more information on the pixel instead of just the abs value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants