Skip to content
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

Redesign color arithmetic #131

Merged
merged 9 commits into from
Jan 21, 2021
Merged

Redesign color arithmetic #131

merged 9 commits into from
Jan 21, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 20, 2020

This notably defines 3 multiplication operators for RGB colors. It also un-defines abs2, because how that should work is a bit ambiguous. Finally, it defines a new varmult function, which allows one to compute variance using a specific multiplication operator.

Fixes #126. Requires the following:

src/ColorVectorSpace.jl Outdated Show resolved Hide resolved
Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many ddls at the end of the semester, haven't got the time to read the source codes. I just made some documentation suggestions here.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/ColorVectorSpace.jl Outdated Show resolved Hide resolved
This notably defines 3 multiplication operators for RGB colors.
It also un-defines `abs2`, because how that should work is a bit ambiguous.
Finally, it defines a new `varmult` function, which allows one to
compute variance using a specific multiplication operator.

There are some compatibility definitions for current releases of ColorTypes.

Co-authored-by: Johnny Chen <[email protected]>
@codecov
Copy link

codecov bot commented Jan 16, 2021

Codecov Report

Merging #131 (d61e363) into master (757d52e) will increase coverage by 18.71%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #131       +/-   ##
===========================================
+ Coverage   78.86%   97.58%   +18.71%     
===========================================
  Files           2        2               
  Lines         194      207       +13     
===========================================
+ Hits          153      202       +49     
+ Misses         41        5       -36     
Impacted Files Coverage Δ
src/ColorVectorSpace.jl 97.94% <100.00%> (+19.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 757d52e...d61e363. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Jan 16, 2021

I've made this work with current releases; except for the hasmethod and related calls, test coverage is now essentially 100%. (On master it's 79%.)

I am leaning towards calling this 0.9, rather than 1.0, and see how things work for a few months.

@timholy timholy merged commit 66dda94 into master Jan 21, 2021
@timholy timholy deleted the teh/mathrules branch January 21, 2021 22:18
@timholy
Copy link
Member Author

timholy commented Jan 21, 2021

That was a long delay! Good to have this finally done.

eps(::Type{Gray{T}}) where {T} = Gray(eps(T))

for f in (:trunc, :floor, :round, :ceil)
@eval $f(::Type{T}, g::Gray) where {T<:Integer} = Gray{T}($f(T, gray(g)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an over-loose type annotation to me, since we don't really have Gray{<:Integer} types except for Bool.

https://github.com/JuliaGraphics/ColorTypes.jl/blob/0003cf95fc1ed348490d2a467ee95afea55ee9e0/src/types.jl#L344-L346

Making it

$f(::Type{T}, g::Gray) where {T<:Integer} = $f(T, gray(g)))

is more reasonable if we consider that T specifies the output type for such usages.

cref: JuliaGraphics/ColorTypes.jl#123 (comment)

cc: @myrddin89

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

Successfully merging this pull request may close these issues.

Important decisions with respect to color math (please comment)
3 participants