Per-channel contrast #153
Per-channel contrast #153
Conversation
|
Looks like it's still happening. In case it's helpful you can still get an error on master (#151) if you use julia 0.6.4 on our cannon server with this package directory: |
timholy
force-pushed
the
Cody-G-cjg/color_contrast2
branch
5 times, most recently
from
e6ea5ef to
4766f54
Compare
timholy
force-pushed
the
Cody-G-cjg/color_contrast2
branch
from
4766f54 to
b8f4d8a
Compare
src/ImageView.jl
Outdated
| @@ -476,11 +476,11 @@ function histsignals(enabled::Signal, defaultimg, img::Signal, clim::Signal{CLim | |||
| return histsigs | |||
| end | |||
|
|
|||
| function scaleminmax(mtyp::DataType, cmin::AbstractRGB{T}, cmax::AbstractRGB{T}) where {T} | |||
| function scaleminmax(::Type{Tout}, cmin::AbstractRGB{T}, cmax::AbstractRGB{T}) where {T,Tout} | |||
There was a problem hiding this comment.
This change makes this function inferrable. Since it runs on every pixel, that's important. A DataType type is typically a recipe for non-inferrability.
There was a problem hiding this comment.
I think this brings out a longstanding point of confusion for me. Kind of amazing that I still don't get this right. I thought that annotating mtyp in this case was only for readability / catching errors and did not affect inferrability. I thought that as long as the compiler can infer the type of mtyp and all operations within the scaleminmax are themselves type stable then the entire scaleminmax function should be type stable. But am I correct that data types are fundamentally non-inferrable and for this reason the Type{xxx} construct was invented to wrap a datatype and make it inferrable?
There was a problem hiding this comment.
Right, types-as-arguments are in a bit of a special category. You can think of arguments as tuples, and type-elements of tuples "lose" their type info:
julia> typeof((1,"hello",Int))
Tuple{Int64,String,DataType}There was a problem hiding this comment.
Ah this makes sense now, thanks! And it's helpful to know that args are just Tuples in case I forget this again.
src/ImageView.jl
Outdated
| r = scaleminmax(T, red(cmin), red(cmax)) | ||
| g = scaleminmax(T, green(cmin), green(cmax)) | ||
| b = scaleminmax(T, blue(cmin), blue(cmax)) | ||
| return x->mtyp(r(red(x)), g(green(x)), b(blue(x))) | ||
| return x->Tout(nanz(r(red(x))), nanz(g(green(x))), nanz(b(blue(x)))) |
There was a problem hiding this comment.
This fixes an error when displaying this test image.
src/ImageView.jl
Outdated
| @@ -504,13 +504,16 @@ function prep_contrast(img::Signal, clim::Signal{CLim{T}}) where {T} | |||
| # Return a signal corresponding to the scaled image | |||
| imgc = map(img, clim; name="clim-mapped image") do image, cl | |||
| cmin, cmax = safeminmax(cl.min, cl.max) | |||
| mtyp = T<:GrayLike ? Gray{N0f8} : T | |||
| smm = scaleminmax(mtyp, cmin, cmax) | |||
| smm = scaleminmax(outtype(T), cmin, cmax) | |||
There was a problem hiding this comment.
This may not be strictly necessary, but it coerces images into a display format (N0f8 storage) which catches some problems earlier and uses less memory than, e.g., RGB{Float64}.
|
Thank you for your patience---and especially, your contribution---@Cody-G! |
|
Thanks for chasing down this nasty error and getting it merged! |
ghost
mentioned this pull request
This is a tiny change on top of #145, which will hopefully get it passing tests. CC @Cody-G