Made `WeightVec` a subtype of `RealVector` #248

rofinn · 2017-04-20T22:08:35Z

Seems like WeightVec should support the same functionality as regular vectors.

ararslan · 2017-04-20T22:28:09Z

src/weights.jl

@@ -281,6 +288,9 @@ Base.mean{T<:Number,W<:Real}(A::AbstractArray{T}, w::WeightVec{W}, dim::Int) =


 ###### Weighted median #####
+function Base.median(v::AbstractArray, w::WeightVec)


Shouldn't this method be unnecessary since all you're doing is throwing a MethodError? Presumably without the method, it would also be a MethodError.

I needed to added this because this test was failing with a BoundsError instead. It appears that making WeightVec a RealVector allowed it to dispatch to a method in base.

julia> median([4 3 2 1 0], weights(wt)) ERROR: BoundsError: attempt to access 2-element Array{Any,1} at index [3] in mapslices(::Base.#median!, ::Array{Int64,2}, ::StatsBase.WeightVec{Int64,Array{Int64,1},Int64}) at ./abstractarray.jl:1619 in median(::Array{Int64,2}, ::StatsBase.WeightVec{Int64,Array{Int64,1},Int64}) at ./statistics.jl:579``` I figured it was better to manually throw a `MethodError` rather than letting it hit the `BoundsError` in that case.

Ah, I see. This is fine then. Thanks for clarifying.

That said, the current behavior isn't that different:

julia> median([4 3 2 1 0], weights([1,2,3,4,5])) ERROR: MethodError: no method matching start(::StatsBase.WeightVec{Int64,Array{Int64,1}}) Closest candidates are: start(::SimpleVector) at essentials.jl:259 start(::Base.MethodList) at reflection.jl:560 start(::ExponentialBackOff) at error.jl:107 ... Stacktrace: [1] append_any(::StatsBase.WeightVec{Int64,Array{Int64,1}}, ::Vararg{StatsBase.WeightVec{Int64,Array{Int64,1}},N} where N) at ./essentials.jl:170 [2] median(::Array{Int64,2}, ::StatsBase.WeightVec{Int64,Array{Int64,1}}) at ./statistics.jl:619

So I'm not sure this MethodError is really needed. We typically don't throw them for all possible cases that fail.

ararslan · 2017-04-20T22:28:56Z

It would be great if you could add tests for this.

nalimilan · 2017-04-21T07:15:10Z

src/weights.jl

-immutable WeightVec{W,Vec<:RealVector}
-    values::Vec
-    sum::W
+immutable WeightVec{T, V<:RealVector, S} <: RealVector{T}


Better do WeightVec{S<:Real, T<:Real, V<:AbstractVector{T}} <: AbstractVector{T}. That should simplify constructors since the parameter could be inferred from the input arguments.

EDIT: RealVector is just an alias used for conciseness, but it's not useful when defining types since it's redundant with the information provided by T<:Real.

Is it really useful to allow typeof(sum) and eltype(values) to be different? Having three parameters for such a simple type seems overly complicated.

I wondered about this too, then I realized that it might be useful to allow storing weights using a small type (UInt8, Float16) to save space, and yet be able to store the sum in a type that won't overflow or lose precision too easily (Int64, BigInt, Float64).

My view was that all of these could be useful in the cases like WeightVec{Bool, NullableArray{Bool}, Int}, but I figured you'd be more likely to want to dispatch on V than S.

@nalimilan Isn't WeightVec{S<:Real, T<:Real, V<:AbstractVector{T}} <: AbstractVector{T} triangular dispatch which is only supported in 0.6?

@nalimilan I've updated the parameterization to match your suggestion, but I needed to version check whether we can use triangular dispatch (implemented in this PR).

- Parameterization to `WeightVec{S<:Real, T<:Real, T<:AbstractVector}` - Uses triangular dispatch for julia version passed v"0.6.0-dev.2123"

nalimilan · 2017-04-22T14:22:37Z

src/weights.jl

+    end
+
+    function WeightVec{T<:Real, V<:AbstractVector{T}}(vs::V)
+        sum_ = sum(vs)


The trailing underscore is a bit weird. Why not use s or any other simple name?

This is mostly just habit as I try to avoid using single character variable names. For math and stats code I tend to use the name of the function name with an underscore as my variable names if I can't think of anything better.

nalimilan · 2017-04-22T14:24:39Z

src/weights.jl

+        sum::S
+    end
+
+    function WeightVec{S<:Real, T<:Real, V<:AbstractVector{T}}(vs::V, s::S)


You should be able to move this function and the next one out of the version-dependent block using V<:RealVector and replacing T with eltype(vs). That is, just slightly adapting the existing constructors.

nalimilan · 2017-04-23T15:54:19Z

src/weights.jl

 end

 """
-    WeightVec(vs, [wsum])
+    function WeightVec{S<:Real, V<:RealVector}(vs::V, s::S)


This change is incorrect, the signature should just reflect how the user can call the function, not how it's implemented.

nalimilan · 2017-04-23T15:55:39Z

src/weights.jl


 Construct a `WeightVec` with weight values `vs` and sum of weights `wsum`.
 If omitted, `wsum` is computed.
 """
-WeightVec{Vec<:RealVector,W<:Real}(vs::Vec,wsum::W) = WeightVec{W,Vec}(vs, wsum)
-WeightVec(vs::RealVector) = WeightVec(vs, sum(vs))
+function WeightVec{S<:Real, V<:RealVector}(vs::V, s::S)


You can actually use s::S=sum(vs) to get rid of the next method.

Interesting, I didn't realize that was valid syntax in 0.5 (ie: referencing an earlier argument when setting the default value).

nalimilan · 2017-04-24T08:13:55Z

src/weights.jl

@@ -281,6 +290,9 @@ Base.mean{T<:Number,W<:Real}(A::AbstractArray{T}, w::WeightVec{W}, dim::Int) =


 ###### Weighted median #####
+function Base.median(v::AbstractArray, w::WeightVec)
+    throw(MethodError(median, (typeof(v), typeof(w))))


Hadn't noticed that one. Should pass (v, w) instead of their types. Otherwise, looks good to me.

…ec)`

rofinn · 2017-04-24T17:09:45Z

Should I squash my commits or is a more verbose history preferred here?

nalimilan

Thanks! No need for squashing, we can do it from GitHub when merging.

ararslan · 2017-05-04T18:47:23Z

I'm pretty sure this change is what broke DataArrays. Weighted mean on a DataArray is throwing a MethodError when passed an object constructed with weights().

rofinn · 2017-05-04T19:12:31Z

@ararslan Can you post the specific error?

This seems to work.

julia> da = @data([4.0, 2.0, 3.0])
3-element DataArrays.DataArray{Float64,1}:
 4.0
 2.0
 3.0

julia> w = weights(rand(3))
3-element StatsBase.Weights{Float64,Float64,Array{Float64,1}}:
 0.727859
 0.000978015
 0.301205

julia> mean(da, w)
3.7056809658486145

Not the exact same version of StatsBase, but pretty close for this use case.

nalimilan · 2017-05-04T19:23:26Z

Can you post the code which triggers the failure?

ararslan · 2017-05-04T19:36:13Z

From the DataArrays tests:

da1 = DataArray(randn(128))
da2 = DataArray(randn(128))
da1[1:3:end] = NA
da2[1:2:end] = NA
mean(da1, weights(da2))

The last line is now a MethodError:

julia> mean(da1, weights(da2))
ERROR: MethodError: no method matching StatsBase.WeightVec(::DataArrays.DataArray{Float64,1}, ::DataArrays.NAtype)
Closest candidates are:
  StatsBase.WeightVec(::V<:(AbstractArray{T,1} where T<:Real)) where V<:(AbstractArray{T,1} where T<:Real) at /Users/alex/.julia/v0.7/StatsBase/src/weights.jl:23
  StatsBase.WeightVec(::V<:(AbstractArray{T,1} where T<:Real), ::S<:Real) where {S<:Real, V<:(AbstractArray{T,1} where T<:Real)} at /Users/alex/.julia/v0.7/StatsBase/src/weights.jl:23
Stacktrace:
 [1] weights(::DataArrays.DataArray{Float64,1}) at /Users/alex/.julia/v0.7/StatsBase/src/weights.jl:31

rofinn · 2017-05-04T19:39:05Z

@ararslan That makes sense cause DataArray.NAtype isn't a subtype of Real and sum(da2) returns NA.

rofinn · 2017-05-04T19:40:17Z

Removing the restriction S<:Real would probably fix this.

nalimilan · 2017-05-04T19:41:26Z

OK. I guess we could remove the S<:Real restriction since we don't really need it. But I'm not actually sure this is a use case we want to support: when do you want to use a missing weight? One can always use a zero weight to ignore an observation.

ararslan · 2017-05-04T19:42:19Z

Perhaps, but I'm not sure it's worth loosening that restriction; I don't think it makes sense to have a weight vector whose sum is missing.

ararslan · 2017-05-04T19:43:34Z

Perhaps it would make more sense to require that the input to a weight constructor be a Base.Array rather than any AbstractArray{<:Real}?

nalimilan · 2017-05-04T19:45:16Z

Well, there's no point in restricting the type of underlying array. DataArray{T} is cheating by pretending that its eltype is T when it's actually Union{NAtype, T}.

Let's just remove that test from DataArrays if we don't have a use case for it.

rofinn · 2017-05-04T19:45:46Z

@ararslan To answer your specific question, it was initially WeightVec{W,Vec<:RealVector} and this PR changed it to WeightVec{S<:Real, T<:Real, V<:RealArray} as I thought that made more sense. I tend to agree that having a weight vector whose sum is missing doesn't really make sense.

rofinn · 2017-05-04T19:48:06Z

Seems like a better approach for that use case would be to use dropna before?

ararslan · 2017-05-04T19:49:48Z

Seems like a better approach for that use case would be to use dropna before this?

Not necessarily, since you can dropna from your weight vector but you may have reason for legitimately missing values in your data vector, in which case you don't want to dropna from that, and then the vectors have differing lengths, which is an error.

ararslan · 2017-05-04T19:51:14Z

Let's just remove that test from DataArrays if we don't have a use case for it.

Okay ¯\_(ツ)_/¯

ararslan · 2017-05-04T21:23:38Z

Cool, DataArrays is all good now.

However, it appears this change still introduced ambiguities with Base methods:

julia> using StatsBase

julia> using Base.Test

julia> detect_ambiguities(StatsBase, Base, Core)
Skipping StatsBase.hist
Skipping StatsBase.wmean!
Skipping Base.<|
3-element Array{Tuple{Method,Method},1}:
 (stdm(v::AbstractArray{T,N} where N where T<:Real, wv::StatsBase.WeightVec, m::AbstractArray{T,N} where N where T<:Real, dim::Int64) in StatsBase at /Users/alex/.julia/v0.7/StatsBase/src/moments.jl:99, stdm(v::AbstractArray{T,N} where N where T<:Real, m::AbstractArray{T,N} where N where T<:Real, wv::StatsBase.WeightVec, dim::Int64) in StatsBase at deprecated.jl:50)
 (varm(A::AbstractArray{T,N} where N where T<:Real, wv::StatsBase.WeightVec, M::AbstractArray{T,N} where N where T<:Real, dim::Int64) in StatsBase at /Users/alex/.julia/v0.7/StatsBase/src/moments.jl:65, varm(A::AbstractArray{T,N} where N where T<:Real, M::AbstractArray{T,N} where N where T<:Real, wv::StatsBase.WeightVec, dim::Int64) in StatsBase at deprecated.jl:50)
 (stdm(v::AbstractArray{T,N} where N where T<:Real, wv::StatsBase.WeightVec, m::Real) in StatsBase at /Users/alex/.julia/v0.7/StatsBase/src/moments.jl:87, stdm(v::AbstractArray{T,N} where N where T<:Real, m::AbstractArray{T,N} where N where T<:Real, dim::Int64) in StatsBase at /Users/alex/.julia/v0.7/StatsBase/src/moments.jl:98)

rofinn · 2017-05-04T21:34:28Z

Shouldn't the first two go away once those deprecations are removed? Not sure about the last one though.

ararslan · 2017-05-04T21:35:53Z

Yeah the first two aren't too worrisome, it's mostly the last one. This previously wasn't a problem because we didn't have WeightVec <: AbstractVector. Not sure what to do about it now.

rofinn · 2017-05-04T21:39:30Z

Could we do something like Base.stdm(v::RealVector, w::AbstractWeights, m::Real) and Base.stdm(v::RealMatrix, m::RealArray, dim::Int)?

ararslan · 2017-05-04T21:46:20Z

We can't overload a Base function with Base types though; this is the sinful act of type piracy.

rofinn · 2017-05-04T21:48:09Z

All the more reason for it not to be a Base function? :)

rofinn · 2017-05-04T22:00:16Z

Wait also does this even count as type piracy cause we're overloading our own behaviour and not base? If StatsBase wasn't loading these would result in a MethodError.

ararslan · 2017-05-04T22:01:27Z

If we extend a Base function using Base types, regardless of how we define the behavior, we've committed ~~an act of treason~~ type piracy.

rofinn · 2017-05-04T22:03:59Z

Well in that case we're already committing type piracy with Base.stdm(v::RealArray, m::RealArray, dim::Int).

ararslan · 2017-05-04T23:33:57Z

Oh dang you're right. If we get rid of those we should be okay on ambiguities. We should commence... the purge.

nalimilan · 2017-05-05T09:01:00Z

Indeed it's surprising that we ship this method. It's not even documented in StatsBase. Let's remove it, the ambiguities due to deprecations are OK (though they shouldn't be hard to fix if we wanted to).

rofinn · 2017-05-05T09:58:38Z

I think it makes sense to either keep that method or add it to Base which already has a varm(A::AbstractArray{T,N} where N, m::AbstractArray, region; corrected), but no corresponding method for stdm.

nalimilan · 2017-05-05T10:03:15Z

I guess we could add it to Base. But since it's not exported, it's not really an issue whether a method is missing or not.

rofinn · 2017-05-05T10:20:41Z

It is exported though. https://github.com/JuliaLang/julia/blob/98649ed087d910540bd9c311a4e650a74b5c54f6/base/exports.jl#L861

nalimilan · 2017-05-05T12:14:43Z

Ah, cool. I extrapolated incorrectly from covm, which isn't exported.

ararslan · 2017-05-05T17:29:35Z

That wouldn't help us for 0.5 and 0.6 though, since if we add the method to Base then it will only be available in 0.7+.

ararslan · 2017-05-05T17:31:16Z

We can just define an internal function that behaves like Base.stdm with the appropriate methods, assuming the currently pirated method is being used in any significant way in the package. Otherwise it could just be a simple refactoring.

rofinn · 2017-05-05T17:51:54Z

As far as i can tell it's only being used here, so we could probably rename it to _stdm and change that one reference for now and deprecate the previous method in case anyone was using it. I still think the correct long term solution would be to move the base stats stuff into statsbase, but that sounds like a reasonable interim solution.

ararslan · 2017-05-05T18:09:25Z

Not worth a deprecation IMO since it's a Base function.

nalimilan · 2017-05-06T12:53:59Z

Certainly not worth a deprecation. I think the easiest solution is to define the method on older Julia versions using a VERSION check (or a method_exists one).

Made WeightVec a subtype of RealVector

dc17308

ararslan reviewed Apr 20, 2017

View reviewed changes

Added a few tests to weights.jl

0248bac

ararslan approved these changes Apr 20, 2017

View reviewed changes

ararslan requested a review from nalimilan April 20, 2017 23:27

nalimilan reviewed Apr 21, 2017

View reviewed changes

Changed parameterization of WeightVec.

3cfbc21

- Parameterization to `WeightVec{S<:Real, T<:Real, T<:AbstractVector}` - Uses triangular dispatch for julia version passed v"0.6.0-dev.2123"

nalimilan reviewed Apr 22, 2017

View reviewed changes

Moved constructors out of version check.

5535cad

nalimilan reviewed Apr 23, 2017

View reviewed changes

Cleaned up WeightVec constructors.

f4bffc4

nalimilan reviewed Apr 24, 2017

View reviewed changes

Fixed thrown MethodError call in `median(::AbstractArray, ::WeightV…

e34ebb4

…ec)`

nalimilan approved these changes Apr 24, 2017

View reviewed changes

ararslan merged commit 5a7fa11 into JuliaStats:master Apr 24, 2017

ararslan mentioned this pull request May 4, 2017

Remove a problematic test case that we do not want to support JuliaStats/DataArrays.jl#249

Merged

rofinn mentioned this pull request May 18, 2017

Adding support for different weight vector types #250

Merged

12 tasks

ararslan mentioned this pull request May 18, 2017

Bug or Feature: fit(Histogram,...) and DataFrames #266

Open

rofinn mentioned this pull request May 24, 2017

Ambiguity detection #269

Open

		@@ -281,6 +288,9 @@ Base.mean{T<:Number,W<:Real}(A::AbstractArray{T}, w::WeightVec{W}, dim::Int) =


		###### Weighted median #####
		function Base.median(v::AbstractArray, w::WeightVec)

Made WeightVec a subtype of RealVector #248

Made WeightVec a subtype of RealVector #248

Conversation

rofinn commented Apr 20, 2017

Choose a reason for hiding this comment

rofinn Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Apr 20, 2017

nalimilan Apr 21, 2017 • edited Loading

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

rofinn Apr 21, 2017 • edited Loading

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

rofinn commented Apr 24, 2017

nalimilan left a comment

Choose a reason for hiding this comment

ararslan commented May 4, 2017 • edited Loading

rofinn commented May 4, 2017 • edited Loading

nalimilan commented May 4, 2017

ararslan commented May 4, 2017

rofinn commented May 4, 2017

rofinn commented May 4, 2017

nalimilan commented May 4, 2017

ararslan commented May 4, 2017

ararslan commented May 4, 2017

nalimilan commented May 4, 2017

rofinn commented May 4, 2017

rofinn commented May 4, 2017 • edited Loading

ararslan commented May 4, 2017

ararslan commented May 4, 2017

ararslan commented May 4, 2017

rofinn commented May 4, 2017

ararslan commented May 4, 2017

rofinn commented May 4, 2017

ararslan commented May 4, 2017

rofinn commented May 4, 2017 • edited Loading

rofinn commented May 4, 2017 • edited Loading

ararslan commented May 4, 2017

rofinn commented May 4, 2017 • edited Loading

ararslan commented May 4, 2017

nalimilan commented May 5, 2017

rofinn commented May 5, 2017

nalimilan commented May 5, 2017

rofinn commented May 5, 2017 • edited Loading

nalimilan commented May 5, 2017

ararslan commented May 5, 2017

ararslan commented May 5, 2017

rofinn commented May 5, 2017

ararslan commented May 5, 2017

nalimilan commented May 6, 2017

Made `WeightVec` a subtype of `RealVector` #248

Made `WeightVec` a subtype of `RealVector` #248

rofinn Apr 20, 2017 •

edited

Loading

nalimilan Apr 21, 2017 •

edited

Loading

rofinn Apr 21, 2017 •

edited

Loading

ararslan commented May 4, 2017 •

edited

Loading

rofinn commented May 4, 2017 •

edited

Loading

rofinn commented May 4, 2017 •

edited

Loading

rofinn commented May 4, 2017 •

edited

Loading

rofinn commented May 4, 2017 •

edited

Loading

rofinn commented May 4, 2017 •

edited

Loading

rofinn commented May 5, 2017 •

edited

Loading