Adds build_histogram(Equalization(),...)#761
Adds build_histogram(Equalization(),...)#761zygmuntszpak wants to merge 1 commit intoJuliaImages:masterfrom
build_histogram(Equalization(),...)#761Conversation
Removed the dependence on `searchsortedlast` from `build_histogram` to improve the performance of histogram construction. Added a fresh implementation of histogram equalization which does not require `_histeq_pixel_rescale`.
| end | ||
|
|
||
| function transform_density!(img::AbstractArray,edges::AbstractArray, histogram::AbstractArray, cdf::AbstractArray) | ||
| o = Base.Order.Forward |
There was a problem hiding this comment.
Forgot to remove this.
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
==========================================
- Coverage 95.85% 95.79% -0.06%
==========================================
Files 10 10
Lines 699 714 +15
==========================================
+ Hits 670 684 +14
- Misses 29 30 +1
Continue to review full report at Codecov.
|
| function adjust_histogram(operation::Equalization, img::AbstractArray{T}, nbins::Integer, minval::RealLike = 0, maxval::RealLike = 1) where {T<:AbstractRGB} | ||
| yiq = convert.(YIQ, img) | ||
| yiq_view = channelview(yiq) | ||
| yeq = adjust_histogram!(Equalization(),view(yiq_view,1,:,:),nbins,minval,maxval) |
There was a problem hiding this comment.
Don't need to declare yeq since adjust_histogram! modifies its argument. I forgot to remove the yeq after refactoring my initial implementation.
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
This looks lovely, many thanks for doing this.
| lb = first(axes(edges,1))-1 | ||
| ub = last(axes(edges,1)) | ||
| first_edge = first(edges) | ||
| step_size = edges.step.hi |
There was a problem hiding this comment.
This seems to assume a TwicePrecision element. But I can construct an AbstractRange as 0:11 or StepRangeLen(0.0, 0.1, 11).
There was a problem hiding this comment.
Thanks. I think I've handled the general case in the new version of the code.
| """ | ||
| function adjust_histogram(operation::Equalization, img::AbstractArray{T}, nbins::Integer, minval::RealLike = 0, maxval::RealLike = 1) where {T<:AbstractRGB} | ||
| yiq = convert.(YIQ, img) | ||
| yiq_view = channelview(yiq) |
There was a problem hiding this comment.
Better would be something like y = map(c->YIQ(c).y, img) (untested).
There was a problem hiding this comment.
I actually need not only the y value, but also the i and q values since I need to reconstruct the original colour image. That's why I convert to YIQ and pass the Y as a view to be equalised.
There was a problem hiding this comment.
Good. But wouldn't it still be better to access the fields you want directly rather than using channelview? To me the meaning of A[i,j].y seems much more evident than A[1,i,j].
| See also: [histmatch](@ref),[clahe](@ref), [build_histogram](@ref) and [adjust_gamma](@ref). | ||
|
|
||
| """ | ||
| function adjust_histogram(operation::Equalization, img::AbstractArray{T}, nbins::Integer, minval::RealLike = 0, maxval::RealLike = 1) where {T<:AbstractRGB} |
There was a problem hiding this comment.
Does it have to be AbstractRGB or does any Color3 suffice?
There was a problem hiding this comment.
Color3 is what we want. I occasionally have trouble determining what the most abstract types are---sorry.
| end | ||
|
|
||
| function build_histogram(img::AbstractArray, edges::AbstractRange) | ||
| Base.has_offset_axes(edges) && throw(ArgumentError("edges must be indexed starting with 1")) |
There was a problem hiding this comment.
Seems generic so not sure this is necessary (but I have no objection to including it, either).
There was a problem hiding this comment.
It's not quite generic yet. The manner in which I calculate the index currently still assumes that edges start from 1.
| map!(img,img) do val | ||
| if val >= edges[end] | ||
| newval = cdf[end] | ||
| else |
There was a problem hiding this comment.
do you need to check val < first(edges) here too?
| else | ||
| edges, counts = build_histogram(0:1/255:1,4,128/255,192/255) | ||
| img = collect(0:1/255:1) | ||
| edges, counts = build_histogram(T.(img),4,128/255,192/255) |
There was a problem hiding this comment.
Just curious, what's the reason for this change?
There was a problem hiding this comment.
I wanted to convert the img to all the different types T that we loop over in the outer loop. That was my original intention but I had forgotten to actually convert the img to type T in the previous commit.
| ``` | ||
|
|
||
| Returns a histogram equalised image with a granularity of approximately `nbins` | ||
| number of bins. |
There was a problem hiding this comment.
Can you remind me the reason for the "approximately"?
There was a problem hiding this comment.
This is a copy/paste mistake. I copied the description from imhist. I recently added the word approximately to imhist because imhist doesn't guarantee that the number of bins that the user specifies actually are utilised in the construction of the histogram. Thanks for spotting this---I've fixed it in the latest version.
|
@timholy I got myself a bit tangled with my branching. I've made a new pull-request which updates this work. This pull-requested listed here was from the master branch of my fork. The new pull-request was from a branch I created for this work. Sorry for the confusion. We should probably close this pull-request and move the conversation to the other one. |
|
No problem about the new branch, I know how it goes. I will close this but note some responses above. |
2 participants
Removed the dependence on
searchsortedlastfrombuild_histogramto improve the performance of histogram construction. Added a fresh implementation of histogram equalization which does not require_histeq_pixel_rescale.