Fix error types and conditions in `weighted_color_mean` #421

kimikage · 2020-04-28T07:53:19Z

This fixes #416, but leaves the weight type conversion (#416 (comment)).

Gray{Bool}s do not throw a DomainError due to their type now, although the InexactError is likely to occur. Because of the weight type conversion, the following throws an InexactError.

julia> weighted_color_mean(0.5, Gray{Bool}(1), Gray{Bool}(1))
ERROR: InexactError: Bool(0.5)

The range checking of w1 is done in the type of w1 in order to more intuitive error messages.

Before (master):

julia> weighted_color_mean(-0.001, Gray24(0), Gray24(1))
ERROR: ArgumentError: FixedPointNumbers.Normed{UInt8,8} is an 8-bit type representing 256 values from 0.0 to 1.0; cannot represent -0.001

julia> weighted_color_mean(1.00000005, Gray(0.0f0), Gray(1.0f0)) # Float32(1.00000005) == 1.0f0
Gray{Float32}(0.0f0)

After (this PR):

julia> weighted_color_mean(-0.001, Gray24(0), Gray24(1))
ERROR: DomainError with -0.001:
`w1` must be in [0, 1]

julia> weighted_color_mean(1.00000005, Gray(0.0f0), Gray(1.0f0))
ERROR: DomainError with 1.00000005:
`w1` must be in [0, 1]

I'm not sure wheter this is a breaking change. If this is a breaking change, I think it is better to remove the weight type conversion in this PR.

codecov · 2020-04-28T08:19:29Z

Codecov Report

Merging #421 into master will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
+ Coverage   81.70%   81.97%   +0.26%     
==========================================
  Files          10       10              
  Lines         880      882       +2     
==========================================
+ Hits          719      723       +4     
+ Misses        161      159       -2

Impacted Files	Coverage Δ
src/utilities.jl	`97.10% <100.00%> (+3.07%)`	⬆️

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 6c71c14...5d07301. Read the comment docs.

johnnychen94

The improvement of the test set looks great!

johnnychen94 · 2020-05-09T10:13:41Z

src/utilities.jl

+    weight2 = oneunit(weight1) - weight1
+    mapc((x, y)->convert(T, muladd(weight1, x, weight2 * y)), c1, c2)
+end
+function _weighted_color_mean(w1::Integer, c1::C, c2::C) where C <: Colorant


Any reason not allowing different types of c1 and c2, since it only requires w1 to be 0 or 1.

Suggested change

function _weighted_color_mean(w1::Integer, c1::C, c2::C) where C <: Colorant

function _weighted_color_mean(w1::Integer, c1::Colorant, c2::Colorant)

Any reason allowing different types of c1 and c2? They cannot be optimized, i.e. can throw errors.

I don't get it, why would it throw errors? Is that because c1 and c2 are not promoted to a common type?

Is that because c1 and c2 are not promoted to a common type?

Yes. Of course, no error should be thrown for colors in the same color space, but this optimization is not available.

julia> weighted_color_mean(1, Gray{Float32}(0), Gray{Float64}(1)) Gray{Float64}(0.0)

johnnychen94 · 2020-05-09T10:33:29Z

src/utilities.jl

-    mapc((x,y)->weight1*x+weight2*y, c1, c2)
+    @fastmath min(w1, oneunit(w1) - w1) >= zero(w1) || throw(DomainError(w1, "`w1` must be in [0, 1]"))
+    T = promote_type(eltype(c1), eltype(c2))
+    weight1 = convert(T, w1) # TODO: Consider the need for this


I guess we should make the following work without throwing error?

weighted_color_mean(0.5, Gray{Bool}(0), Gray{Bool}(0))

I noticed this issue when tackling the problem with Gray{Bool}, but I think RGB{N0f8} is more important for people involved in image processing.

I mean this could be a good reason to remove convert(T, w1) 😃

I agree with you. However, I don't understand the purpose of the conversion, so I can't come to a conclusion. Whatever the reason, if we remove it, it should be in v0.13.

I'd agree it can be removed. That seems like a pretty minor breakage (in accordance with the viewpoint that "types are an implementation detail, values are what matter"), so I'd be OK either with v0.12 or v0.13.

The removal requires some modifications of tests, so I think it's better to make a separate PR. If we make the two steps anyway, it's a bit safer to do this in v0.12 and the removal in v0.13.

timholy

LGTM, sorry I forgot about this!

timholy · 2020-06-08T08:58:52Z

src/utilities.jl

-    mapc((x,y)->weight1*x+weight2*y, c1, c2)
+    @fastmath min(w1, oneunit(w1) - w1) >= zero(w1) || throw(DomainError(w1, "`w1` must be in [0, 1]"))
+    T = promote_type(eltype(c1), eltype(c2))
+    weight1 = convert(T, w1) # TODO: Consider the need for this


I'd agree it can be removed. That seems like a pretty minor breakage (in accordance with the viewpoint that "types are an implementation detail, values are what matter"), so I'd be OK either with v0.12 or v0.13.

kimikage · 2020-06-08T10:24:58Z

Thank you for your review despite your busy schedule, @timholy.
Also, thank you for your comment, @johnnychen94.

I opened a new issue for removing the conversion. I will submit a PR later.

kimikage requested a review from timholy April 30, 2020 16:04

kimikage mentioned this pull request May 9, 2020

Version 0.12.1 #423

Merged

kimikage force-pushed the weighted_mean branch from f1b7156 to f05ab5c Compare May 9, 2020 02:51

johnnychen94 reviewed May 9, 2020

View reviewed changes

Fix error types and conditions in weighted_color_mean

5d07301

kimikage force-pushed the weighted_mean branch from f05ab5c to 5d07301 Compare June 8, 2020 05:55

timholy approved these changes Jun 8, 2020

View reviewed changes

kimikage merged commit 53c2acb into JuliaGraphics:master Jun 8, 2020

kimikage deleted the weighted_mean branch June 8, 2020 10:03

kimikage mentioned this pull request Jun 8, 2020

Stopping forced conversion of weight type in weighted_color_mean #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error types and conditions in `weighted_color_mean` #421

Fix error types and conditions in `weighted_color_mean` #421

kimikage commented Apr 28, 2020

codecov bot commented Apr 28, 2020 •

edited

Loading

johnnychen94 left a comment

johnnychen94 May 9, 2020

kimikage May 9, 2020

johnnychen94 May 9, 2020

kimikage May 9, 2020

johnnychen94 May 9, 2020

kimikage May 9, 2020

johnnychen94 May 9, 2020

kimikage May 9, 2020

timholy Jun 8, 2020

kimikage Jun 8, 2020

timholy left a comment

timholy Jun 8, 2020

kimikage commented Jun 8, 2020

	function _weighted_color_mean(w1::Integer, c1::C, c2::C) where C <: Colorant
	function _weighted_color_mean(w1::Integer, c1::Colorant, c2::Colorant)

Fix error types and conditions in weighted_color_mean #421

Fix error types and conditions in weighted_color_mean #421

Conversation

kimikage commented Apr 28, 2020

Before (master):

After (this PR):

codecov bot commented Apr 28, 2020 • edited Loading

Codecov Report

johnnychen94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimikage commented Jun 8, 2020

Fix error types and conditions in `weighted_color_mean` #421

Fix error types and conditions in `weighted_color_mean` #421

codecov bot commented Apr 28, 2020 •

edited

Loading