LinearStretching enhancements #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
==========================================
+ Coverage 95.81% 95.86% +0.05%
==========================================
Files 14 14
Lines 454 484 +30
==========================================
+ Hits 435 464 +29
- Misses 19 20 +1
Continue to review full report at Codecov.
|
|
great! in what cases doesn't it work for N0f8? |
The kernel operation of out .= (b-a)/(B-A) * (X - (A*b-B*a)/(b-a))where This expression says if Paremeters The simplest solution is to do My opinion is to:
|
|
this issue should exist for all fixed-point types not just N0f8 IIUC.
however, i see your point. give the user the flexibility to stretch outside [0,1] and then clamp themselves if that is what they desire. and if they stretch into a type which can't support values outside [0,1], then either throw an error or promote. maybe make clamping easy for them by having a |
|
Recent changes (including 0572eb4):
Performance tweak get ~10x speed up: julia> img = testimage("mandril_gray");
julia> alg = LinearStretching();
# master
julia> @benchmark adjust_histogram($img, $alg)
BenchmarkTools.Trial:
memory estimate: 256.19 KiB
allocs estimate: 3
--------------
minimum time: 4.473 ms (0.00% GC)
median time: 4.739 ms (0.00% GC)
mean time: 4.804 ms (0.03% GC)
maximum time: 7.016 ms (0.00% GC)
--------------
samples: 1041
evals/sample: 1
# this PR
julia> @benchmark adjust_histogram($img, $alg)
BenchmarkTools.Trial:
memory estimate: 256.14 KiB
allocs estimate: 2
--------------
minimum time: 493.738 μs (0.00% GC)
median time: 535.095 μs (0.00% GC)
mean time: 550.216 μs (0.36% GC)
maximum time: 2.837 ms (80.10% GC)
--------------
samples: 9068
evals/sample: 1More benchmark on this PR: julia> img = testimage("mandril_gray");
julia> alg = LinearStretching();
julia> @btime adjust_histogram($img, $alg);
483.575 μs (2 allocations: 256.14 KiB)
julia> @btime adjust_histogram($(float.(img)), $alg);
880.483 μs (2 allocations: 1.00 MiB)
julia> alg = LinearStretching(src_minval=0.1, src_maxval=0.9);
# clamp contributes ~200μs for N0f8
julia> @btime adjust_histogram($img, $alg);
670.129 μs (2 allocations: 256.14 KiB)
# but doesn't for Float32
julia> @btime adjust_histogram($(float.(img)), $alg);
894.220 μs (2 allocations: 1.00 MiB) |
johnnychen94
commented
Mar 19, 2020
johnnychen94
changed the title
|
@zygmuntszpak I think this PR is ready for review/merge, commits are rebased into two. Summary:
|
bjarthur
reviewed
Mar 20, 2020
src/algorithms/linear_stretching.jl
Outdated
| @@ -2,7 +2,7 @@ | |||
| """ | |||
| ``` | |||
| LinearStretching <: AbstractHistogramAdjustmentAlgorithm | |||
| LinearStretching(; minval = 0, maxval = 1) | |||
| LinearStretching(; minval = 0, maxval = 1, [src_minval], [src_maxval]) | |||
There was a problem hiding this comment.
for symmetry, maybe minval and maxval should be dst_minval and dst_maxval ?
There was a problem hiding this comment.
I've thought about that once a while, then I think it's okay to keep the previous minval, maxval name since those are what most user needs.
And I guess adding another deprecation after imadjustintensity is kind of annoying to users...
There was a problem hiding this comment.
i'm not sure about "most users needs". i for one would only ever need to set src_{min,max}val. worth noting that matlab's equivalent function uses symmetrically named keyword args ({low,high}_{in,out}). i would also argue that naming them dst_{min,max}val would be more self-descriptive and hence act as documentation, both in the user-facing API and in the code.
There was a problem hiding this comment.
Yes, I agree and the re-naming makes sense to me as well. I was just concerning how welcome/unwelcome this renaming will be.
I'm open to changes, but I'd like to first see how @zygmuntszpak thinks before I put my hands on it.
There was a problem hiding this comment.
I agree that it is best to rename it to dst_minval and dst_maxval. Presumably, this means that we will have to amend the version number to become 0.4.0 to reflect the breaking change?
There was a problem hiding this comment.
Presumably, this means that we will have to amend the version number to become 0.4.0 to reflect the breaking change?
If we're on 1.x.y version, a minor version bump is needed. But since we're on 0.3.4 version, I think a patch version is also accepted.
When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place
https://semver.org/#how-should-i-handle-deprecating-functionality
There was a problem hiding this comment.
In addition to the deprecation of minval, maxval in favor of dest_minval, dest_maxval in a198016, I also introduced a new set of API in 5674568
- specify (dest_minval, dest_maxval):
LinearStretching(nothing=>(0, 1)) - specify (src_minval, src_maxval):
LinearStretching((0.1, 0.9)) - specify both:
LinearStretching((0.1, 0.9) => (0, 1))
src/algorithms/linear_stretching.jl
Outdated
| @@ -2,7 +2,7 @@ | |||
| """ | |||
| ``` | |||
| LinearStretching <: AbstractHistogramAdjustmentAlgorithm | |||
| LinearStretching(; minval = 0, maxval = 1) | |||
| LinearStretching(; minval = 0, maxval = 1, [src_minval], [src_maxval]) | |||
There was a problem hiding this comment.
I agree that it is best to rename it to dst_minval and dst_maxval. Presumably, this means that we will have to amend the version number to become 0.4.0 to reflect the breaking change?
* expose src_minval and src_maxval of LinearStretching ( issue #27) * performance tweak by * avoiding recomputation in inner loop * early return for trivial case * clamp values to (minval, maxval)
* LinearStretching((0.1, 0.8)=>(0.2, 0.9)) * LinearStretching(nothing=>(0.2, 0.9)) * LinearStretching((0.1, 0.8))
Although two new fields are introduced to LinearStretching, it doesn't hurt the general performance; the construction time is about 3ns
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
|
Hmmm, |
johnnychen94
commented
Mar 21, 2020
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
|
Benchmark after 65c7c22:
julia> img = testimage("mandril_gray");
julia> alg = LinearStretching();
julia> @btime adjust_histogram($img, $alg);
265.302 μs (6 allocations: 1.25 MiB)
julia> @btime adjust_histogram($(float.(img)), $alg);
909.417 μs (2 allocations: 1.00 MiB)
julia> @btime adjust_histogram($(Float32.(img)), $alg);
879.758 μs (2 allocations: 1.00 MiB)
julia> alg = LinearStretching((0.1f0, 0.9f0));
julia> @btime adjust_histogram($img, $alg);
275.747 μs (6 allocations: 1.25 MiB)
julia> @btime adjust_histogram($(float.(img)), $alg);
882.314 μs (2 allocations: 1.00 MiB)
julia> @btime adjust_histogram($(Float32.(img)), $alg);
880.484 μs (2 allocations: 1.00 MiB)I think it's ready to merge (with squash) if there aren't more suggestions. The color version might need more tweaks As a reference, here's the performance of julia> img = testimage("mandril_gray");
julia> @btime imadjustintensity($img);
1.146 ms (4 allocations: 256.20 KiB)
julia> @btime imadjustintensity($(float.(img)));
1.175 ms (4 allocations: 1.00 MiB)
julia> @btime imadjustintensity($img, (0.1, 0.9));
605.017 μs (3 allocations: 256.17 KiB)
julia> @btime imadjustintensity($(float.(img)), (0.1, 0.9));
341.675 μs (3 allocations: 1.00 MiB) 👀 travis CI failures on macos are unrelated. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/ImageContrastAdjustment.jl/ImageContrastAdjustment.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
although clamp before linear stretching is slightly faster, it has boundary rounding issues, which causes test failures and isn't reliable to use
zygmuntszpak
approved these changes
Mar 24, 2020
bjarthur
reviewed
Mar 24, 2020
src/algorithms/linear_stretching.jl
Outdated
| # this is only used when user specifies custom `(src_minval, src_maxval)` | ||
| out_minval = r * img_min - o | ||
| out_maxval = r * img_max - o | ||
| do_clamp = (out_minval < dst_minval) || (out_maxval > dst_maxval) |
There was a problem hiding this comment.
this PR is great as is, and is all i (at least currently) need. however, making the clamp optional would provide for maximum flexibility. would this be possible with a keyword argument, or otherwise?
thanks for all the hard work @johnnychen94
There was a problem hiding this comment.
In the first two commits, it's hard to handle this correctly wrt FixedPoint, and then I totally forget about this. Sorry about that.
I've added a no_clamp option in a254af4
|
I didn't expect this PR going so long, so here's a summary:
I plan to merge this in two days if there are no further comments |
|
@zygmuntszpak @bjarthur thanks for the insightful suggestions on the API |
|
Nice work @johnnychen94! |
|
indeed, great work! sorry i didn't think to suggest these earlier, but two things:
i can submit a PR if there is a consensus about either one or the other or both. |
|
Although it doesn't make a real performance difference for I'm okay to rename it to |
|
@bjarthur Could you please elaborate on how specifying a vector rather than a tuple is more convenient? I would prefer not to support two different ways of specifying the same thing.
|
|
i believe in julia it is standard to accept both 2-tuples and 2-vectors when specifying the dimensions along which to perform an algebraic operation. so for example with similarly for with LinearStretching in some cases the min/max vals you want to use might be calculated and stored in a vector and it is a hassle for the user to |
That could be quite a fair use case for me. I think it's okay to get this since I believe it doesn't get things worse in this case. However, I'm not excited to add this to the codebase; I kind of prefer to distinguish tuple and vector and to not mixing them. |
|
@bjarthur Perhaps you could open an issue on the Images repo regarding your suggestion of also allowing one to pass a vector instead of a tuple. If we do this here, then I would like it to become a consensus for the Images ecosystem. I want to avoid a situation where some key functions do this kind of conversion automatically, and others not. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's just a prototype for #27 , does this usage look good to you?
Note that this implementation doesn't always work for
N0f8types, there're two ways to work this around: clamp01 or promote storage type. I'm not sure which is better.cc: @zygmuntszpak @bjarthur