-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use IndirectArrays #47
Conversation
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
==========================================
- Coverage 90.99% 88.31% -2.68%
==========================================
Files 14 15 +1
Lines 222 231 +9
==========================================
+ Hits 202 204 +2
- Misses 20 27 +7
Continue to review full report at Codecov.
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Let's try to output indices |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Comparing yesterday's (Y) commits with today's (T): Benchmark comparison
So per-channel dithering generally got a little slower and
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
I'm happy with color algorithms and error diffusion in general getting faster. I'd be happy to merge this PR, especially if we can find some way to make ordered dithering run faster than before! |
We could also use something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a confirmation: do we still have the in-place version of dither!
and binarydither!
?
Yes, but it calls the regular non-inplace algorithm and simply writes over the image afterwards: DitherPunk.jl/src/dither_api.jl Line 40 in a433a2b
If you want to compare the performance, the benchmarks for
No, all We could use IndirectArrays for the color algorithms only, however this would be a little hacky when doing per-channel dithering, which calls I also think the package is more transparent for the user if every output is an IndirectArray. |
Can you explain this a bit more? I’m not sure if I understand it. Do you mean you ideally want to make |
I should not have used the word hacky, let's take a look at both options: This PRIn this PR, every call to Currently DitherPunk does three things:
Working with indices and IndirectArrays...
Per-channel dithering is in the middle as it uses The alternativeThe alternative would have been to separate binary and color algorithms:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is slower for binary dithering as we now need to allocate a new array of indices.
Per-channel dithering is in the middle as it uses binarydither on each channel but makes good use of IndirectArrays. Therefore it got less of a slow-down in the benchmarks.
Sounds valid argument to me to still use normal Array
for binarydither
. Maybe we should still provide the in-place version of binarydither
so we can reuse the buffer. #47 (comment)
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Previously got: Warning: Assignment to `d` in soft scope is ambiguous because a global variable by the same name exists: `d` will be treated as a new local. Disambiguate by using `local d` to suppress this warning or `global d` to assign to the existing global variable.
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
@johnnychen94 IndirectArrays are now only used when dithering in a color palette. I reverted binary dithering and per-channel dithering to use in-place updates. dither(img, FloydSteinberg()) |> IndirectArray Ordered dithering appears to be around the same speed as previously while dropping the TiledIteration dependency. I'm not sure how to resolve the Julia 1.0 dependency conflict. |
I guess we need to update the hot-fix in CI setup DitherPunk.jl/.github/workflows/CI.yml Lines 47 to 50 in a7d6ff1
Pkg.add([
PackageSpec(name="Images", version="0.24"),
PackageSpec(name="IndirectArrays", version="0.5"),
PackageSpec(name="ImageCore", version="0.8"),
]) |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
Sorry for the spam, I forgot this was re-running the benchmarks in here... 😬 |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Baseline resultBenchmark Report for /home/runner/work/DitherPunk.jl/DitherPunk.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 versioninfo
Runtime information
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Merge when you're ready.
Thanks for the help! 😊 |
Closes #28
All internal dithering algorithms (
binarydither
,colordither
) now return index matrices for construction of IndirectArrays indither
.For
binarydither
,I've left the code to return 0 for black and 1 for white, as I thought the indices 1 and 2 would be too confusing.we output const indicesINDEX_BLACK
andINDEX_WHITE
.Adds dependency on IndirectArrays, removes dependency on TiledIteration.