Skip to content

Broken optimization of Normed multiplication and inefficient multiplication #166

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

Closed
kimikage opened this issue Apr 27, 2021 · 0 comments · Fixed by #167
Closed

Broken optimization of Normed multiplication and inefficient multiplication #166

kimikage opened this issue Apr 27, 2021 · 0 comments · Fixed by #167

Comments

@kimikage
Copy link
Collaborator

As you know, Normed multiplication in FixedPointNumbers v0.8 is slow. The performance will be improved in FixedPointNumbers v0.9 (cf. JuliaMath/FixedPointNumbers.jl#213), but there are still some issues to be solved before FixedPointNumbers v0.9 is released.

As mentioned below, the real scaling for RGBs is made faster by specialization for Normed. On the other hand, the multiplication for grays and the new multiplication operator use the slow multiplication.

julia> using BenchmarkTools, ColorVectorSpace, ColorTypes, FixedPointNumbers, ColorBlendModes;

julia> a = rand(RGB{N0f8}, 1000, 1000); b = rand(RGB{N0f8}, 1000, 1000);

julia> a .⊙ b == BlendMultiply.(a, b) # the same operation
true

julia> @btime $a .⊙ $b;
  10.084 ms (2 allocations: 2.86 MiB)

julia> @btime BlendMultiply.($a, $b);
  881.000 μs (2 allocations: 2.86 MiB)

One possible option is just to wait for FixedPointNumbers v0.9, and it's okay for me.
However, there is a problem with the current RGB optimization. The following is a bug which has existed since the initial working commit of ColorVectorSpace.

function (*)(f::Normed, c::AbstractRGB{T}) where T<:Normed
fs = reinterpret(f)*(1/widen(reinterpret(oneunit(T)))^2)
rettype(*, f, c)(fs*reinterpret(red(c)), fs*reinterpret(green(c)), fs*reinterpret(blue(c)))
end

julia> 0.5N0f8 * RGB{N0f16}(1, 0.5, 0)
RGB{N0f16}(0.00195,0.00098,0.0)

For that reason, I would like to rewrite the implementation of the multiplication, not so much for the performance, but for the code quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant