-
Notifications
You must be signed in to change notification settings - Fork 0
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
Improve conversions and support ImageCore #4
Conversation
These are required for compatibility with ImageView
Codecov Report
@@ Coverage Diff @@
## main #4 +/- ##
==========================================
+ Coverage 62.33% 67.07% +4.73%
==========================================
Files 5 6 +1
Lines 77 82 +5
==========================================
+ Hits 48 55 +7
+ Misses 29 27 -2
Continue to review full report at Codecov.
|
This is partly based on the doctest failure
While it seems simple, I ended up changing my mind about how things work here several times. For that reason I'll leave this open a bit in case anyone feels like reviewing it. The current behavior is to throw errors upon conversion to RGB if overflow occurs; you can circumvent this by using |
docs/Project.toml
Outdated
[compat] | ||
Documenter = "0.27" |
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.
If we don't need fancy features, then it's perhaps a good idea to just drop the compat. I'm okay either way.
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.
Mostly it's to avoid breakage if Documenter 0.28 or higher breaks some things we use, but that hasn't happened in quite some time so...:shrug:
I'll change it.
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.
The current behavior is to throw errors upon conversion to RGB if overflow occurs; you can circumvent this by using convert(RGB{T}, c) for some type T which can support the full range of values (e.g., Float32).
It seems quite clearly documented in README, I believe it's also more stable and safer to be more restrictive for the first few versions.
src/types.jl
Outdated
@@ -69,8 +69,8 @@ ColorMixture{T}(Cs::NTuple{N,AbstractRGB}, channels::Vararg{Real,N}) where {T,N} | |||
@inline promote_typeof(obj1, obj2, objs...) = _promote_type(typeof(obj1), typeof(obj2), objs...) | |||
|
|||
computeT(Cs::NTuple{N,AbstractRGB}, channels::NTuple{N,Real}) where {N} = eltype(promote_typeof(map(*, Cs, channels)...)) | |||
ColorMixture(Cs::NTuple{N,AbstractRGB}, channels::NTuple{N,Real}) where {N} = ColorMixture{computeT(Cs, channels)}(Cs, channels) | |||
ColorMixture(Cs::NTuple{N,AbstractRGB}, channels::Vararg{Real,N}) where {N} = ColorMixture{computeT(Cs, channels)}(Cs, channels) | |||
Base.@constprop :aggressive ColorMixture(Cs::NTuple{N,AbstractRGB}, channels::NTuple{N,Real}) where {N} = ColorMixture{computeT(Cs, channels)}(Cs, channels) |
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 this the optimization that reduces the generalization overhead you found in #2 (comment)?
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.
No, in this case the performance hit was about 100x. But that penalty was clearly dependent on specifics of the implementation since as stated in the previous comment it seems to have gone away.
The issue basically is that the RGB values are part of the type, and if we change the values in some way (e.g., RGB{N0f8} -> RGB{Float32}
) then unless the values are aggressively const-propped then inference may not be able to calculate them. But now all operations return an element of the same type as the original, by default, so that seems to fix things.
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.
In other words, sometimes inference needs to be able to anticipate this change:
julia> C1 = typeof(ColorMixture(channels))
ColorMixture{N0f8, 2, (RGB{N0f8}(0.0, 0.925, 0.365), RGB{N0f8}(1.0, 0.859, 0.0))}
julia> C2 = typeof(ColorMixture(float.(channels)))
ColorMixture{Float32, 2, (RGB{Float32}(0.0, 0.9254902, 0.3647059), RGB{Float32}(1.0, 0.85882354, 0.0))}
and inference is not capable of that on 1.6. I added this point to the README.
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.
And the bigger story is that I'm now supporting 1.6. I think the caution is still important, though.
But put a caveat in the README
Here's an interesting thought: what if the RGB colors are always |
I thought by parameterizing on values |
Currently the type declaration is struct ColorMixture{T,N,Cs} <: Color{T,N}
channels::NTuple{N,T}
Compat.@constprop :aggressive function ColorMixture{T,N,Cs}(channels::NTuple{N}) where {T,N,Cs}
Cs isa NTuple{N,RGB{T}} || throw(TypeError(:ColorMixture, "incompatible color types", NTuple{N,RGB{T}}, typeof(Cs)))
return new{T,N,Cs}(channels)
end
end so there's an inner-constructor check that |
Right, I was assuming that |
OK, I'll make that change and then OK to merge? |
These are required for compatibility with ImageView.
Two important notes: