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

Adds Gamma Correction Function #492

Merged
merged 5 commits into from
Jul 21, 2016
Merged

Conversation

mronian
Copy link
Contributor

@mronian mronian commented Jun 6, 2016

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.03%) to 74.695% when pulling 6f3100e on mronian:gamma-correction into f50b32b on timholy:master.


"""

RET_TYPE = Dict("8bit" => Gray{U8}, "16bit" => Gray{FixedPointNumbers.UFixed{UInt16,16}})
Copy link
Member

Choose a reason for hiding this comment

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

Any time you pass type information via a string, you're going to have type-instability. I let this slide on the histogram equalization PR, but we should get away from that pattern ASAP. It's much better to pass the type as a type.

@timholy timholy added the needs tests Requires additional tests before it can be merged. label Jun 14, 2016
@bjarthur
Copy link
Collaborator

what are we going to do with imstretch, which performs "symmetric gamma-correction" ?

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-0.4%) to 74.416% when pulling 8ec10eb on mronian:gamma-correction into 919f480 on timholy:master.

@mronian
Copy link
Contributor Author

mronian commented Jul 13, 2016

@timholy I have made the function pixel-wise and the code has reduced by a lot (just a line for color images, the chain of functions is a bit long though). Like histeq, the dtype causes the return type to be marked red with @code_warntype. Also, map() seem to be marked red ( they return a Union, eg Union{Array{Float64,2},Array{Union{},2}}). Is this an error?

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-0.4%) to 74.416% when pulling 044ec95 on mronian:gamma-correction into 919f480 on timholy:master.

function adjust_gamma{T<:FixedPointNumbers.UFixed, D<:Union{U8, U16}}(img::AbstractArray{Gray{T}}, gamma::Number, dtype::Type{D} = U8)
raw_type = FixedPointNumbers.rawtype(dtype)
gamma_inv = 1.0 / gamma
table = [dtype((i / typemax(raw_type)) ^ gamma_inv) for i in 0:typemax(raw_type)]
Copy link
Member

Choose a reason for hiding this comment

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

This line is one source of type instability. It can be replicated with

The type instability can be replicated with
```jl
function prep_table{D}(::Type{D}, gamma_inv)
    raw_type = FixedPointNumbers.rawtype(D)
    [D((i / typemax(raw_type)) ^ gamma_inv) for i in zero(raw_type):typemax(raw_type)]
end

But put a D in front of the comprehension, and suddenly inference is happier. Probably JuliaLang/julia#15276.

@timholy
Copy link
Member

timholy commented Jul 15, 2016

If you're interested in pursuing this further, it would be good to address @bjarthur's question too.

@mronian
Copy link
Contributor Author

mronian commented Jul 17, 2016

@bjarthur We could have a function adjust_gamma which takes in a method for symmetric gamma correction as an argument. The imstretch function has matrix dot style operations and needs to be changed anyways to a pixel wise one. Would it be better to do it as a part of adjust_gamma or a separate function is better?

@mronian
Copy link
Contributor Author

mronian commented Jul 20, 2016

Made a lot of changes, simplified adjust_gamma on the lines of #516 and tests for the same. Tests fail because isapprox for ARGB, AGray is not merged/tagged(JuliaGraphics/ColorVectorSpace.jl#47).

@mronian
Copy link
Contributor Author

mronian commented Jul 20, 2016

There is still some type instability issue with map-

julia> function foo(i)
           i*i
       end
foo (generic function with 1 method)

julia> img = reshape(1:1:100, 10, 10)
10×10 Base.ReshapedArray{Int64,2,StepRange{Int64,Int64},Tuple{}}:
  1  11  21  31  41  51  61  71  81   91
  2  12  22  32  42  52  62  72  82   92
  3  13  23  33  43  53  63  73  83   93
  4  14  24  34  44  54  64  74  84   94
  5  15  25  35  45  55  65  75  85   95
  6  16  26  36  46  56  66  76  86   96
  7  17  27  37  47  57  67  77  87   97
  8  18  28  38  48  58  68  78  88   98
  9  19  29  39  49  59  69  79  89   99
 10  20  30  40  50  60  70  80  90  100

julia> @code_warntype map(foo, img)
Variables:
  #self#::Base.#map
  f::#foo
  A::Base.ReshapedArray{Int64,2,StepRange{Int64,Int64},Tuple{}}

Body:
  begin  # abstractarray.jl, line 1478:
      return $(Expr(:invoke, _collect(::Base.ReshapedArray{Int64,2,StepRange{Int64,Int64},Tuple{}}, ::Base.Generator{Base.ReshapedArray{Int64,2,StepRange{Int64,Int64},Tuple{}},#foo}, ::Base.EltypeUnknown, ::Base.HasShape), :(Base._collect), :(A), :($(Expr(:new, Base.Generator{Base.ReshapedArray{Int64,2,StepRange{Int64,Int64},Tuple{}},#foo}, :(f), :(A)))), :($(Expr(:new, :(Base.EltypeUnknown)))), :($(Expr(:new, :(Base.HasShape))))))
  end::Union{Array{Int64,2},Array{Union{},2}}

The end line is marked red.

@mronian mronian mentioned this pull request Jul 21, 2016
@timholy
Copy link
Member

timholy commented Jul 21, 2016

That's been one of the most long-standing problems in julia: the possibility that the input would be an empty array made type-stable map impossible, because map needed at least one element in order to compute the type of the output.

Having been frustrated by this issue for at least 2 years now (e.g., JuliaLang/julia#8027), I'm very pleased to report that...if you update your julia, you'll see this has (very recently) been fixed! There were some deep-guts improvements that made it finally possible to use type-inference to reliably infer the output type of foo even if there were no actual elements to play with.

on the input type. If the input is an `Image` then the resulting image is of the same type
and has the same properties.

For coloured images, the input is converted to YCbCr type and the Y channel is gamma corrected.
Copy link
Member

Choose a reason for hiding this comment

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

YIQ

@timholy
Copy link
Member

timholy commented Jul 21, 2016

Aside from this tiny edit, LGTM. Seems to need a rebase before merging, however.

@mronian
Copy link
Contributor Author

mronian commented Jul 21, 2016

Done :) Tests fail due to isapprox.

@timholy timholy merged commit 84134b3 into JuliaImages:master Jul 21, 2016
@mronian mronian deleted the gamma-correction branch July 24, 2016 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Requires additional tests before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants