updates to ones, zeros, oneunit and zero to keep consistency to Base #175

johnnychen94 · 2020-03-01T08:37:12Z

make usage like ones(Gray{N0f8}, 4, 4) doesn't throw depwarns (closes support ones(::Type{<:AbstractGray}, sz) #137)
(RFC) make oneunit(::Gray) and zero(::Gray) valid and keep consistency to Base behavior
eltype(zeros(Gray)) == typeof(zero(Gray)) == Gray{N0f8} (closes promote_rule for Type{Any} Colors.jl#369)

There must be some discussion on these in julialang/julia but I can't find a source...

codecov · 2020-03-01T08:47:19Z

Codecov Report

Merging #175 into master will increase coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   92.06%   92.18%   +0.11%     
==========================================
  Files           7        7              
  Lines         668      678      +10     
==========================================
+ Hits          615      625      +10     
  Misses         53       53

Impacted Files	Coverage Δ
src/traits.jl	`96.37% <0.00%> (-0.17%)`	⬇️
src/show.jl	`100.00% <0.00%> (ø)`	⬆️

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 03b029e...ac7c586. Read the comment docs.

With `one` deprecated in favor of `oneunit`, the fallback implementation of `ones` would complain about the usage of `one`. This commit specializes on `ones(Gray, args...)` so that it doesn't throw warnings.

This keep consistency to Base, e.g., `zero(0.8) == 0` and `oneunit(0.8) == 1`

kimikage

Except for UndefVarError: g not defined, I think there is no problem. I think that the return type of ones(Gray) should be decided in terms of image processing.

Edit:
Some of the added methods will be unnecessary when one(::Type{<:Gray}) switch to returning 1. It might be nice to have TODO comments.

test/traits.jl

and also fixes some dispatch issues on range type

johnnychen94

Some of the added methods will be unnecessary when one(::Type{<:Gray}) switch to returning 1

🤔 not very sure which method would become unnecessary here. If you could elaborate more I'm glad to add more comments

johnnychen94 · 2020-03-01T12:56:27Z

test/traits.jl

-        @test zero(g) == zero(T) == Gray(zero(T))
-    end
-
-    @test_broken one(Gray{Float32}) * g == g * one(Gray{Float32}) == g


ColorVectorSpaces seems already fixes this, so it not a valid test anymore.

The test means one(Gray) should return 1. Do not remove it now.

Neither Gray{N0f8}(1) * Gray{N0f8}(0.8) nor 1 * Gray{N0f8}(0.8 are valid without ColorVectorSpaces...

And Gray{N0f8} == 1 is true

The test means one(Gray) should return 1

I'll add one more test for this.

Neither Gray{N0f8}(1) * Gray{N0f8}(0.8) nor 1 * Gray{N0f8}(0.8 are valid without ColorVectorSpaces...

You are right. However, I think the broken test makes sense to indicate that "this is broken" and "one is an identity element".

johnnychen94 · 2020-03-01T13:02:25Z

The latest commit also fixes JuliaGraphics/Colors.jl#369

kimikage · 2020-03-01T13:40:25Z

not very sure which method would become unnecessary here.

I had assumed that eltype(ones(Gray)) === Gray. We will probably need almost all of the added methods. 😄

and add missing test for `one`

kimikage · 2020-03-01T13:59:23Z

src/traits.jl

+# specialize ones and zeros for Gray type:
+#   * Base implementation uses `one` instead of `oneunit`; however, `one(Gray)` will return `1`
+#   * Base implementation use `Array{T}` to initialize the array, however, if we pass
+#     `Gray`, it gives an abstract eltype `Array{Gray}`, which is inconsistent to `zero(Gray)`


To be honest, I don't understand what this comment says. I feel the reason is the opposite.
Even though one(Gray) should return 1, not so. Thus, one shows the warning. Isn't this PR a workaround to avoid that warning? If one(Gray) returns 1, ones(Gray) returns an array of Gray(1).

Regarding the latter:

julia> zero(AbstractFloat) 0.0 julia> zeros(AbstractFloat) 0-dimensional Array{AbstractFloat,0}: 0.0

Gray{N0f8} is just a matter of practicality, i.e. speed or memory efficiency.

I meant to make ones(Gray) still return an array of Gray(1) regardless of the fact that one(Gray) === 1 in the future. Did I misunderstand the original reason for the shift?

I think I need to raise zeros(AbstractFloat) issue at julialang/julia.. My idea is to make the array eltype equals(===) to its initializer, which makes more sense to me.

I think I need to raise zeros(AbstractFloat) issue at julialang/julia..

Umm... What is the problem?

That eltype(ones(AbstractFloat)) === typeof(one(AbstractFloat)) should be true (and for zeros)

Yes. I understand the claim, but I just want to know the reason.

Edit:
ones should return ones instead of oneunits as the name suggests.
If we follow your claim, ones should throw an error because of typeof(one(Gray{T}))!== Gray{T} (in the near future).

Just opened a PR for this JuliaLang/julia#34948

wrt whether ones(Gray{N0f8}, 4, 4) should return array of N0f8 or Gray{N0f8}, I'd like to get @timholy involved. My consideration is that ones is used so widely that we might want to abuse the meaning for image processing purpose.

Definitely, ones(Gray{N0f8}, 4, 4) should not return an array of N0f8. Where did the choice come from?

ones(Gray{N0f8}, 2, 2) --> Gray{N0f8}[? ? ? ?] --> Gray{N0f8}[one(Gray{N0f8}) one(Gray{N0f8}) one(Gray{N0f8}) one(Gray{N0f8})] --> Gray{N0f8}[1 1 1 1] --> Gray{N0f8}[Gray{N0f8}(1) Gray{N0f8}(1) Gray{N0f8}(1) Gray{N0f8}(1)]

ones(Gray, 2, 2) --> Gray[? ? ? ?] --> Gray[one(Gray) one(Gray) one(Gray) one(Gray)] --> Gray[1 1 1 1] --> Gray[convert(Gray, 1) convert(Gray, 1) convert(Gray, 1) convert(Gray, 1)] --> Gray[Gray{N0f8}(1) Gray{N0f8}(1) Gray{N0f8}(1) Gray{N0f8}(1)]

As you know, the array with non-concrete element type has the performance issue. I think your suggestions are good for application specific types. But in general that is a "premature optimization".

timholy · 2020-03-01T17:40:57Z

zeros should be fine but that already seems to work properly.

ones is more problematic in that I don't think ones should return anything different from an array of one(T) but the distinction between one and oneunit causes trouble. xref JuliaGraphics/Colors.jl#369 (comment) and JuliaLang/julia#34948 (comment).

johnnychen94 · 2020-03-02T01:12:21Z

Since we have different rules for Gray and Number, wouldn't that be more surprising when ones returns an array of Number while its siblings zeros returns an array of Gray?

Also, what benefits would one(Gray) be by saying it returns a multiplicative identity for Gray when we disallow the multiplication...except that users don't get MethodError when they call ones?

I can understand the general rules here but it looks like I'm still keeping confused when using these in the wild; it's very hard to not thinking zeros/ones as the counterpart of that in Matlab and NumPy worlds.

Said this, I would feel a lot better if one returns the same type as zero, whatever it might be. (For Base consistency or for general image processing convenience)

johnnychen94 · 2020-03-02T01:28:08Z

Let's not include this PR in v0.10 and leave more time to think about it.

timholy · 2020-03-02T01:39:18Z

The point is that if one is supposed to be the multiplicative identity, it needs to at least be that. So we have to decide whether Gray(1)*Gray(1) == Gray(1). Currently it's not unless you load ColorVectorSpace. And for RGB the situation is even more confusing.

Really, ones should just die and be replaced by fill.

johnnychen94 · 2020-03-02T01:49:46Z

I believe it's harmless to support 0.8 * Gray(1.) and Gray(1.) * 0.8 here, which can be read as "multiply the grayscale by 0.8"? (but not Gray(1) * Gray(1)). This makes 1 a valid multiplicative identity for Gray.

Really, ones should just die and be replaced by fill.

This becomes the most unambiguous solution for me now.

timholy · 2020-03-02T08:55:38Z

This makes 1 a valid multiplicative identity for Gray

If we support multiplication here at all. From a standpoint of pure colorimetry I'm not certain that multiplying RGB by anything other than 1 makes sense. (Of course we have to support it in image processing...)

kimikage · 2020-03-03T04:40:25Z

I don't think ones should return anything different from an array of one(T)

Originally posted by @timholy in #175 (comment)

I think ones(T, ...) should return an array of the first argument T, because ones (and zeros) should be for the "array constructor".
If one(T) should not be converted to T (e.g. Dates.Day), ones should throw an error. However,
both a Gray value and 1 can be converted to Gray.
There is disagreement about whether that is appropriate, but at least, the current implementation agrees with that (cf. #175 (comment)).

julia> ones(AbstractFloat,2,2)
2×2 Array{AbstractFloat,2}:
 1.0  1.0
 1.0  1.0

Of course, one(AbstractFloat) == 1.0, so this is not a good example, though. I mean:

julia> Base.one(::Type{AbstractFloat}) = 1 # `1` should be a multiplicative identity for `AbstractFloat`, too.

julia> one(AbstractFloat)
1

julia> ones(AbstractFloat, 2,2)
2×2 Array{AbstractFloat,2}:
 1.0  1.0
 1.0  1.0

julia> Base.one(::Type{AbstractFloat}) = true # I think this is somewhat strange, but ok.

julia> oneunit(AbstractFloat)
1.0

julia> ones(AbstractFloat, 2,2)
2×2 Array{AbstractFloat,2}:
 1.0  1.0
 1.0  1.0

We can get the same results for concrete types (e.g. Float64), since AbstractFloat is a supertype of all floating-point types.

Whether ones(Gray, ...) should return an array of Gray{N0f8} is another matter. I don't care about it. (I think throwing error is also reasonable.)

timholy · 2020-03-12T10:31:02Z

I'm converging on the following strategy:

delete one(Gray{T}) from this package altogether (one(Gray{T}) will throw an error if you've only loaded ColorTypes)
In ColorVectorSpace, define one(Gray{T}) as Gray{T}(1). That's the opposite of what our depwarn here indicates, but we'd be using the longstanding existence of the depwarn as an excuse for step 1 above.
Move this PR to ColorVectorSpace

kimikage · 2020-03-12T10:37:47Z

It looks reasonable to me. I'm sure some people will wonder why ColorTypes doesn't define one, though. 😄

timholy · 2020-03-12T10:47:21Z

I am beginning to think we need to extend Julia's printing of MethodErrors to allow packages to push custom hints. Similar to JuliaLang/julia#34642 and many similar changes, but package-extensible.

EDIT: JuliaLang/julia#35094

kimikage · 2021-05-14T16:39:01Z

The issues related to this PR have been settled by PR #243.

Now, ones(Gray, ...), zeros(RGB, ...) etc. are not recommended for the performance reason.

julia> ones(Gray{N0f8}, 4, 4)
4×4 Matrix{Gray{N0f8}}:
 1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0
 1.0  1.0  1.0  1.0

julia> ones(Gray, 4, 4)
4×4 Matrix{Gray}:
 Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)
 Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)
 Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)
 Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)  Gray{N0f8}(1.0)

johnnychen94 added 2 commits March 1, 2020 16:56

specialize ones for Gray

940a996

With `one` deprecated in favor of `oneunit`, the fallback implementation of `ones` would complain about the usage of `one`. This commit specializes on `ones(Gray, args...)` so that it doesn't throw warnings.

don't error for zero(::Gray) and oneunit(::Gray)

c05e5df

This keep consistency to Base, e.g., `zero(0.8) == 0` and `oneunit(0.8) == 1`

kimikage reviewed Mar 1, 2020

View reviewed changes

test/traits.jl Outdated Show resolved Hide resolved

test/traits.jl Outdated Show resolved Hide resolved

test/traits.jl Outdated Show resolved Hide resolved

test/traits.jl Show resolved Hide resolved

rewrite tests on zeros and ones seriously

cc2e3e1

and also fixes some dispatch issues on range type

johnnychen94 commented Mar 1, 2020

View reviewed changes

johnnychen94 changed the title ~~updates to ones, oneunit and zero to keep consistency to Base~~ updates to ones, zeros, oneunit and zero to keep consistency to Base Mar 1, 2020

make one(::Gray) valid as well

ac7c586

and add missing test for `one`

kimikage reviewed Mar 1, 2020

View reviewed changes

kimikage mentioned this pull request Mar 1, 2020

promote_rule for Type{Any} JuliaGraphics/Colors.jl#369

Closed

kimikage mentioned this pull request Mar 8, 2020

Version 0.10.0 #176

Merged

johnnychen94 mentioned this pull request Apr 20, 2020

Do not merge: Implement < as well as isless, zero & oneunit for more types #186

Closed

kimikage mentioned this pull request Mar 12, 2021

[RFC] Decision on the definition of one #235

Closed

kimikage closed this May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to ones, zeros, oneunit and zero to keep consistency to Base #175

updates to ones, zeros, oneunit and zero to keep consistency to Base #175

johnnychen94 commented Mar 1, 2020 •

edited

Loading

codecov bot commented Mar 1, 2020 •

edited

Loading

kimikage left a comment •

edited

Loading

johnnychen94 left a comment •

edited

Loading

johnnychen94 Mar 1, 2020

kimikage Mar 1, 2020

johnnychen94 Mar 1, 2020

kimikage Mar 1, 2020

johnnychen94 commented Mar 1, 2020

kimikage commented Mar 1, 2020

kimikage Mar 1, 2020 •

edited

Loading

johnnychen94 Mar 1, 2020 •

edited

Loading

kimikage Mar 1, 2020

johnnychen94 Mar 1, 2020 •

edited

Loading

kimikage Mar 1, 2020 •

edited

Loading

johnnychen94 Mar 1, 2020 •

edited

Loading

kimikage Mar 1, 2020 •

edited

Loading

timholy commented Mar 1, 2020

johnnychen94 commented Mar 2, 2020

johnnychen94 commented Mar 2, 2020

timholy commented Mar 2, 2020

johnnychen94 commented Mar 2, 2020 •

edited

Loading

timholy commented Mar 2, 2020

kimikage commented Mar 3, 2020 •

edited

Loading

timholy commented Mar 12, 2020

kimikage commented Mar 12, 2020

timholy commented Mar 12, 2020 •

edited

Loading

kimikage commented May 14, 2021

updates to ones, zeros, oneunit and zero to keep consistency to Base #175

updates to ones, zeros, oneunit and zero to keep consistency to Base #175

Conversation

johnnychen94 commented Mar 1, 2020 • edited Loading

codecov bot commented Mar 1, 2020 • edited Loading

Codecov Report

kimikage left a comment • edited Loading

Choose a reason for hiding this comment

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

johnnychen94 Mar 1, 2020

Choose a reason for hiding this comment

kimikage Mar 1, 2020

Choose a reason for hiding this comment

johnnychen94 Mar 1, 2020

Choose a reason for hiding this comment

kimikage Mar 1, 2020

Choose a reason for hiding this comment

johnnychen94 commented Mar 1, 2020

kimikage commented Mar 1, 2020

kimikage Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

johnnychen94 Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

kimikage Mar 1, 2020

Choose a reason for hiding this comment

johnnychen94 Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

kimikage Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

johnnychen94 Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

kimikage Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

timholy commented Mar 1, 2020

johnnychen94 commented Mar 2, 2020

johnnychen94 commented Mar 2, 2020

timholy commented Mar 2, 2020

johnnychen94 commented Mar 2, 2020 • edited Loading

timholy commented Mar 2, 2020

kimikage commented Mar 3, 2020 • edited Loading

timholy commented Mar 12, 2020

kimikage commented Mar 12, 2020

timholy commented Mar 12, 2020 • edited Loading

kimikage commented May 14, 2021

johnnychen94 commented Mar 1, 2020 •

edited

Loading

codecov bot commented Mar 1, 2020 •

edited

Loading

kimikage left a comment •

edited

Loading

johnnychen94 left a comment •

edited

Loading

kimikage Mar 1, 2020 •

edited

Loading

johnnychen94 Mar 1, 2020 •

edited

Loading

johnnychen94 Mar 1, 2020 •

edited

Loading

kimikage Mar 1, 2020 •

edited

Loading

johnnychen94 Mar 1, 2020 •

edited

Loading

kimikage Mar 1, 2020 •

edited

Loading

johnnychen94 commented Mar 2, 2020 •

edited

Loading

kimikage commented Mar 3, 2020 •

edited

Loading

timholy commented Mar 12, 2020 •

edited

Loading