Add handling of an empty iterator for mean and var by bkamins · Pull Request #29033 · JuliaLang/julia

bkamins · 2018-09-04T09:34:58Z

This is a follow up of #28777 fixing what is minimally needed to be consistent in Julia 1.0 between empty arrays and empty collections (needed for skipmissing).

Major cleanup of consistency of the outputs for different scenarios of application of functions from Statistics module are left for Julia 2.0.

nalimilan · 2018-09-04T12:01:17Z

stdlib/Statistics/test/runtests.jl

    @test ismissing(mean([missing, NaN]))
    @test isequal(mean([missing 1.0; 2.0 3.0], dims=1), [missing 2.0])
    @test mean(skipmissing([1, missing, 2])) === 1.5
+    @test isequal(mean(Complex{Float64}[]), NaN+NaN*im)


It would also make sense to test that an empty Int input gives a Float64 output, and that an error is thrown when the element type isn't known (e.g. using a generator).

BTW, you could probably use ===.

This PR is in muddy waters :) You cannot use === as we have different NaN in Julia and here it would fail. Notice that:

julia> NaN === -NaN false

and this is exactly the case here, that is why I had to use isequal.

I will add the tests.

I must be missing something: you really expect NaN::Float64 here, right?

The situation is more complex (and that is why I have used this as a test):

julia> x = mean(Complex{Float64}[]) NaN + NaN*im julia> bitstring(x.re) "1111111111111000000000000000000000000000000000000000000000000000" julia> bitstring(x.im) "1111111111111000000000000000000000000000000000000000000000000000" julia> bitstring(NaN) # see that the bitstring is different "0111111111111000000000000000000000000000000000000000000000000000"

also you have:

julia> x = mean(Int[]) NaN julia> x === NaN false

For the same reason that we have different NaNs in Julia (and as you see they actually happen in practice).

nalimilan · 2018-09-04T12:02:42Z

stdlib/Statistics/test/runtests.jl

        @test f(skipmissing([1, missing, 2]), 0) === f([1, 2], 0)
    end
+
+    @test isequal(mean(Complex{Float64}[]), NaN)


I guess you intended to test var?

Sure. Fixed.

nalimilan · 2018-09-04T12:23:56Z

stdlib/Statistics/src/Statistics.jl

    if y === nothing
-        throw(ArgumentError("variance of empty collection undefined: $(repr(iterable))"))
+        T = eltype(iterable)
+        return typeof((abs2(zero(T)) + abs2(zero(T)))/2)(NaN)


So this throws an error when calling zero if eltype returns Any. Maybe better ~~call~~ throw a more explicit error similar to the existing one?

Could use oftype.

I would leave it as is, because this is how var for AbstractArray is implemented and we want to be non-breaking (so it should have 100% the same behavior - the same outputs and the same errors - so using array or iterator is transparent).

For sure in Julia 2.0 approach we could consider cleaning it up in both places.

I just mean that instead of letting zero throw the error, it would be clearer to check T === Any and throw a more explicit error.

The problem is that T does not have to be Any. It can be an arbitrary type that is or is not a subtype of Number. And upfront we do not know if this type:

defines zero

has abs2 defined

implements a constructor from NaN

Any of those may or may not be true so the only way to be consistent is to repeat the same code that is used for a version of var defined for arrays (otherwise I will be able to create a situation where array and iterator var differs).

OK. We could still print a nice error in the most common case, but then we'd lose consistency.

This is the point for now. As discussed in Julia 2.0 we can probably clean it up in both methods.

personally, would use oftype as @nalimilan suggested for clarity

nalimilan · 2018-09-04T15:39:46Z

stdlib/Statistics/test/runtests.jl

    @test mean(skipmissing([1, missing, 2])) === 1.5
+    @test isequal(mean(Complex{Float64}[]), NaN+NaN*im)
+    @test isequal(mean(skipmissing(Complex{Float64}[])), NaN+NaN*im)
+    @test mean(Complex{Float64}[]) isa Complex{Float64}


Why not also test the value? Same in other places.

The value is tested above in line 89 (unless you meant something else).

Sorry, this one is, I originally spotted mean(Int[]). It would probably be clearer to have them side by side.

All cleaned up now (hopefully 😄, as you have a keener eye for details 👍)

nalimilan

Thanks, looks good to me, but another review wouldn't hurt.

bkamins · 2018-09-13T09:42:04Z

One CI error seems unrelated.

StefanKarpinski · 2018-09-13T12:47:49Z

Who would be a good person to review?

nalimilan · 2018-09-13T12:54:45Z

@TotalVerb?

TotalVerb

overall lgtm

There's a mean(::AbstractRange{<:Real}) method on line 134. Might be worth changing that behavior too for consistency, even though ranges can't have missings.

TotalVerb · 2018-09-13T19:16:37Z

stdlib/Statistics/src/Statistics.jl

    if y === nothing
-        throw(ArgumentError("variance of empty collection undefined: $(repr(iterable))"))
+        T = eltype(iterable)
+        return typeof((abs2(zero(T)) + abs2(zero(T)))/2)(NaN)


personally, would use oftype as @nalimilan suggested for clarity

bkamins · 2018-09-14T00:03:47Z

Thank you. Changed to oftype in both places for consistency and changed mean of empty ranges (with added tests).

nalimilan · 2018-09-14T08:03:06Z

stdlib/Statistics/src/Statistics.jl


-function mean(r::AbstractRange{<:Real})
-    isempty(r) && throw(ArgumentError("mean of an empty range is undefined"))
+function Statistics.mean(r::AbstractRange{<:Real})


Why add a module prefix?

Typo. Will fix.

bkamins · 2018-09-28T14:06:19Z

How do we want to proceed with this PR?

nalimilan · 2018-09-28T16:47:31Z

Just merge?

StefanKarpinski · 2018-10-11T18:31:38Z

I think just go ahead with this. We can always revert it.

nalimilan · 2018-10-11T19:02:16Z

I like when everybody approves but nobody actually merges... :-)

…rs).

changes between Julia 1.0 and 1.1, including: - Custom .css-style for compat admonitions. - Information about compat annotations to CONTRIBUTING.md. - NEWS.md entry for PRs #30090, #30035, #30022, #29978, #29969, #29858, #29845, #29754, #29638, #29636, #29615, #29600, #29506, #29469, #29316, #29259, #29178, #29153, #29033, #28902, #28761, #28745, #28708, #28696, #29997, #28790, #29092, #29108, #29782 - Compat annotation for PRs #30090, #30013, #29978, #29890, #29858, #29827, #29754, #29679, #29636, #29623, #29600, #29440, #29316, #29259, #29178, #29157, #29153, #29033, #28902, #28878, #28761, #28708, #28156, #29733, #29670, #29997, #28790, #29092, #29108, #29782, #25278 - Documentation for broadcasting CartesianIndices (#30230). - Documentation for Base.julia_cmd(). - Documentation for colon constructor of CartesianIndices (#29440). - Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix). - Run NEWS-update.jl. Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com> Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including: - Custom .css-style for compat admonitions. - Information about compat annotations to CONTRIBUTING.md. - NEWS.md entry for PRs #30090, #30035, #30022, #29978, #29969, #29858, #29845, #29754, #29638, #29636, #29615, #29600, #29506, #29469, #29316, #29259, #29178, #29153, #29033, #28902, #28761, #28745, #28708, #28696, #29997, #28790, #29092, #29108, #29782 - Compat annotation for PRs #30090, #30013, #29978, #29890, #29858, #29827, #29754, #29679, #29636, #29623, #29600, #29440, #29316, #29259, #29178, #29157, #29153, #29033, #28902, #28878, #28761, #28708, #28156, #29733, #29670, #29997, #28790, #29092, #29108, #29782, #25278 - Documentation for broadcasting CartesianIndices (#30230). - Documentation for Base.julia_cmd(). - Documentation for colon constructor of CartesianIndices (#29440). - Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix). - Run NEWS-update.jl. Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com> Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

add handling of an empty iterator for mean and var

4f9028c

bkamins mentioned this pull request Sep 4, 2018

Mean of an empty collection #28777

Open

nalimilan reviewed Sep 4, 2018

View reviewed changes

bkamins force-pushed the empty_collections branch from b7d48b9 to 79be956 Compare September 4, 2018 14:22

additional tests

40b2ea7

bkamins force-pushed the empty_collections branch from 79be956 to 40b2ea7 Compare September 4, 2018 15:32

nalimilan reviewed Sep 4, 2018

View reviewed changes

further cleanup of tests

9e0b0ba

bkamins force-pushed the empty_collections branch from 720b43b to 9e0b0ba Compare September 4, 2018 16:20

nalimilan approved these changes Sep 4, 2018

View reviewed changes

TotalVerb approved these changes Sep 13, 2018

View reviewed changes

change to oftype and define mean of empty range

0487d17

bkamins force-pushed the empty_collections branch from 0aa0683 to 0487d17 Compare September 14, 2018 01:39

nalimilan reviewed Sep 14, 2018

View reviewed changes

remove module qualifier

bff28ee

StefanKarpinski added the triage This should be discussed on a triage call label Sep 28, 2018

nalimilan merged commit b519147 into JuliaLang:master Oct 11, 2018

nalimilan removed the triage This should be discussed on a triage call label Oct 11, 2018

bkamins deleted the empty_collections branch October 11, 2018 19:20

fredrikekre added a commit that referenced this pull request Nov 30, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

e019c39

…rs).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

2b21aae

…rs).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

a069a3a

…rs).

fredrikekre added a commit that referenced this pull request Dec 3, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

431247e

…rs).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

f6c5b0b

…rs).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29033 (mean and var for empty iterato…

f83333d

…rs).

nalimilan mentioned this pull request Jan 23, 2021

Allow passing 0-length vectors to cor JuliaStats/Statistics.jl#70

Open

Uh oh!

Conversation

bkamins commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalimilan Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalimilan left a comment

Choose a reason for hiding this comment

Uh oh!

bkamins commented Sep 13, 2018

Uh oh!

StefanKarpinski commented Sep 13, 2018

Uh oh!

nalimilan commented Sep 13, 2018

Uh oh!

TotalVerb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkamins commented Sep 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkamins commented Sep 28, 2018

Uh oh!

nalimilan commented Sep 28, 2018

Uh oh!

StefanKarpinski commented Oct 11, 2018

Uh oh!

nalimilan commented Oct 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nalimilan Sep 4, 2018 •

edited

Loading