Add a WeightedResampler#890
Add a WeightedResampler#890rofinn wants to merge 6 commits intoJuliaStats:masterfrom invenia:rf/resample
Conversation
|
The implementation seems good to me but I'm not that familiar with the samplers stuff here. |
Codecov Report
@@ Coverage Diff @@
## master #890 +/- ##
==========================================
+ Coverage 77.74% 77.82% +0.08%
==========================================
Files 112 113 +1
Lines 5370 5390 +20
==========================================
+ Hits 4175 4195 +20
Misses 1195 1195
Continue to review full report at Codecov.
|
|
This would be useful for me :) thanks for adding, Rory! Bump? |
|
Is there anything left to do, before merging this? |
Co-Authored-By: Mathieu Besançon <mathieu.besancon@gmail.com>
|
Bump :) |
|
Sorry about the delay, this looks good to my untrained eyes, and been stalled too long. @rofinn we can merge this after the conflict is resolved |
| test = ["Calculus", "Distributed", "FiniteDifferences", "ForwardDiff", "JSON", "StaticArrays", "Test"] | ||
| test = ["Calculus", "Distances", "Distributed", "FiniteDifferences", "ForwardDiff", "JSON", "StaticArrays", "Test"] | ||
|
|
||
|
|
| wv::AbstractWeights | ||
| end | ||
|
|
||
| function WeightedResampler(obs::T, wv::AbstractWeights) where T<:AbstractArray |
There was a problem hiding this comment.
Is there any reason to restrict weights to AbstractWeights rather than AbstractVector? At JuliaLang/julia#31395, I made functions accept any array since there's no ambiguity.
There was a problem hiding this comment.
Idk but maybe @rofinn does
But also it'd be an easy follow up to loosen it
There was a problem hiding this comment.
Can we merge as is and open an issue about this? It'd be a non breaking change to loosen the constraint in future. Or just loosen it now and fix it if there's a bug report
There was a problem hiding this comment.
I guess merging this as-is is OK. I've noticed that the weights are passed directly to sample, so if we loosen the signature we should also ensure we convert the argument to AbstractWeights, which is what sample expects (currently at least). But I still think loosening this is a good idea, though not a requirement.
|
bump on this :) |
| """ | ||
| struct WeightedResampler{F<:VariateForm, S<:ValueSupport, T<:AbstractArray} <: Sampleable{F, S} | ||
| obs::T | ||
| wv::AbstractWeights |
There was a problem hiding this comment.
Woops, this should be a type parameter!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #890 +/- ##
==========================================
+ Coverage 77.74% 77.82% +0.08%
==========================================
Files 112 113 +1
Lines 5370 5390 +20
==========================================
+ Hits 4175 4195 +20
Misses 1195 1195 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Closing due to inactivity. |
7 participants
Works for all VariateForm and ValueSupport types
The tests should document our use case, but two questions remain.
@testsets internally, but it's unclear if that's the desired test organization for Distributions.jlAliasTabletype, so perhaps we can merge the two... or maybe theWeightedResamplershould use an alias table internally? I believe the current implementation has O(n) runtime and the alias table would give O(1) with an upfront cost of O(n log(n))?