deprecate keyword color_space for ErrorDiffusion #72
Conversation
This is not type inferrable and would thus hurt the performance.
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
- Coverage 98.44% 98.36% -0.09%
==========================================
Files 14 14
Lines 257 244 -13
==========================================
- Hits 253 240 -13
Misses 4 4
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 versioninfoTargetBaselineTarget 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 versioninfoBaseline 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 versioninfoRuntime information
|
|
I'm surprised that this actually makes things slower... Edit: This might be implementation-specific; if the filter weights are |
| @@ -71,11 +73,11 @@ function binarydither!(alg::ErrorDiffusion, out::GenericGrayImage, img::GenericG | |||
| end | |||
|
|
|||
| function colordither( | |||
| alg::ErrorDiffusion{F,C}, | |||
| alg::ErrorDiffusion{C}, | |||
There was a problem hiding this comment.
Shouldn't this be ErrorDiffusion{C,F}?
| @@ -116,7 +118,7 @@ function colordither( | |||
| end | |||
|
|
|||
| """ | |||
| SimpleErrorDiffusion() | |||
| SimpleErrorDiffusion(color_space=XYZ) | |||
There was a problem hiding this comment.
Maybe we can just interpolate a second const docstring similar to _error_diffusion_kwargs into all methods?
| @@ -131,10 +133,11 @@ $(_error_diffusion_kwargs) | |||
| Scale." SID 1975, International Symposium Digest of Technical Papers, | |||
| vol 1975m, pp. 36-37. | |||
| """ | |||
| SimpleErrorDiffusion() = ErrorDiffusion(OffsetMatrix([0 1; 1 0]//2, 0:1, 0:1)) | |||
| SimpleErrorDiffusion(CT=XYZ) = ErrorDiffusion{CT}(_simple_filter) | |||
There was a problem hiding this comment.
This looks like a bug in master, SimpleErrorDiffusion should also pass kwargs... such as clamp_error forward.
| function FloydSteinberg(; kwargs...) | ||
| return ErrorDiffusion(OffsetMatrix([0 0 7; 3 5 1]//16, 0:1, -1:1); kwargs...) | ||
| end | ||
| FloydSteinberg(CT=XYZ; kwargs...) = ErrorDiffusion{CT}(_floydsteinberg_filter; kwargs...) |
There was a problem hiding this comment.
Instead of using the pattern
Method(CT=XYZ; kwargs...) = ErrorDiffusion{CT}(filter; kwargs...)wouldn't it be possible to do
Method(args...; kwargs...) = ErrorDiffusion(filter, args...; kwargs...)using a second ErrorDiffusion constructor
ErrorDiffusion(filter, CT=XYZ; kwargs...) = ErrorDiffusion{CT}(filter; kwargs...)This way, future changes to ErrorDiffusion don't require changes to all other methods.
There was a problem hiding this comment.
Master currently defines
function ErrorDiffusion(filter; color_space=XYZ, clamp_error=true)on L32. Would changing this to
function ErrorDiffusion(filter, color_space=XYZ; clamp_error=true)be enough for type inference?
| return ErrorDiffusion(OffsetMatrix([0 0 7; 3 5 1]//16, 0:1, -1:1); kwargs...) | ||
| end | ||
| FloydSteinberg(CT=XYZ; kwargs...) = ErrorDiffusion{CT}(_floydsteinberg_filter; kwargs...) | ||
| const _floydsteinberg_filter = OffsetMatrix([0 0 7; 3 5 1]//16,0:1,-1:1) |
There was a problem hiding this comment.
This is very nitpicky feedback, but maybe we could call this const FLOYDSTEINBERG_FILTER to be consistent with the constant matrices in ordered.jl.
I've had some shower-thoughts about this. I think we should fully drop the dependency on |
|
Closing this as it is outdated by #73 , which contains parts of this PR. |
This is not type inferrable and would thus hurt the performance.
Changing the order of parameters is technically breaking, but I doubt people outside are depending on it. Thus an appropriate deprecation of
color_spacekeyword should make this a transparent improvement.