Skip to content

Conversation

lbittarello
Copy link
Contributor

@lbittarello lbittarello commented Sep 22, 2019

This pull request:

  • removes the type of the values vector as a parameter of weights;
  • deprecates the values method;
  • adds dims as a keyword argument of sum (in line with the unweighted version).

It addresses issues #358 and #371.

I have not dropped the tests of values yet.

@lbittarello
Copy link
Contributor Author

@nalimilan, we've also discussed the possibility of changing indexing so it returns another weight vector. I've tried this:

@propagate_inbounds function Base.getindex(wv::W, i::Integer) where W <: AbstractWeights
    @boundscheck checkbounds(wv, i)
    v = [wv.values[i]]
    W(v, v)
end

@propagate_inbounds function Base.getindex(wv::W, i::AbstractArray{<:Int}) where W <: AbstractWeights
    @boundscheck checkbounds(wv, i)
    v = wv.values[i]
    W(v, sum(v))
end

Base.getindex(wv::AbstractWeights, ::Colon) = wv

function Base.iterate(it::AbstractWeights, (el, i) = (it.values[1], 0))
	return i >= it.len ? nothing : (el, (it.values[i], i + 1))
end

It works in creating weight vectors from subsets of weight vectors. The iterator seems fine too. But other functionality breaks (for instance, display).

@nalimilan
Copy link
Member

It works in creating weight vectors from subsets of weight vectors. The iterator seems fine too. But other functionality breaks (for instance, display).

Thanks, I had forgotten about this part. We should only define the i::AbstractArray method I think. Indexing with i::Integer should still return a scalar as that's part of the AbstractArray interface.

lbittarello and others added 10 commits September 23, 2019 20:10
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
lbittarello and others added 2 commits September 28, 2019 22:10
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
lbittarello and others added 2 commits September 28, 2019 22:11
@lbittarello
Copy link
Contributor Author

I relaxed the input type of sum (from AbstractArray{<:Number} to AbstractArray) to make up for my previous removal of the fallback method for sum (line 348 of the old code). All tests should go through now.

@lbittarello lbittarello changed the title Simplify weights (#358) Simplify weights Sep 29, 2019
@codecov
Copy link

codecov bot commented Sep 29, 2019

Codecov Report

Merging #526 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #526      +/-   ##
=========================================
+ Coverage   90.47%   90.5%   +0.02%     
=========================================
  Files          21      21              
  Lines        2100    2095       -5     
=========================================
- Hits         1900    1896       -4     
+ Misses        200     199       -1
Impacted Files Coverage Δ
src/deprecates.jl 0% <ø> (ø) ⬆️
src/weights.jl 86.53% <85.71%> (ø) ⬆️
src/moments.jl 100% <0%> (+0.67%) ⬆️

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 95b794a...6f974ea. Read the comment docs.

@lbittarello
Copy link
Contributor Author

Should we harmonize the inputs of sum! and mean!? Since the dimension is a compulsory argument for both, it would make sense to make it a positional instead of a keyword argument in mean!.

@nalimilan
Copy link
Member

Should we harmonize the inputs of sum! and mean!? Since the dimension is a compulsory argument for both, it would make sense to make it a positional instead of a keyword argument in mean!.

Since sum is defined in Base, we should follow what happens there: drop dims, and infer it from the shape of the output array. But I wouldn't spend too much time on this in StatsBase since I've already rewritten a lot of this code as part of JuliaLang/julia#31395. Any changes made here will make it harder to me to rebase that PR on the latest StatsBase.

lbittarello and others added 8 commits September 29, 2019 21:00
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants