ready Accumulator for v1.0 #837
Conversation
Arguably we should just leave this as an ambig error but probably people want it resolved this way
There was a problem hiding this comment.
Hi, folks,
given that I depend on this package, I tried to give a bit back by reviewing. Please feel free to disregard if the community standards permit only reviews from vetted individuals.
The changes look good to me. I had some minor concerns about pop!, which behaves a bit surprisingly to me, but it seems self-consistent. Maybe Counter and Accumulator have different behavior for their pop! in which case everything is fine.
LGTM, but please let me know how I should understand pop!
| @@ -92,7 +90,9 @@ Decrements the count for `x` by `v` (defaulting to one) | |||
| dec!(ct::Accumulator, x, v::Number) = (ct[x] -= v) | |||
| dec!(ct::Accumulator{T,V}, x) where {T,V} = dec!(ct, x, one(V)) | |||
|
|
|||
| #TODO: once we are done deprecating `pop!` for `reset!` then add `pop!` as an alias for `dec!` | |||
There was a problem hiding this comment.
I assume it was decided at some point that pop! is not being deprecated so this TODO is incorrect and that is why it is deleted and disobeyed?
There was a problem hiding this comment.
When pop! becomes an alias for dec! as seen here, then this will be happening:
julia> a = Accumulator(:k=>1); dec!(a, :k); dec!(a, :k)
-1
Just making sure that this is the desired behavior. Asking because I would have expected KeyError when the count goes negative.
| @@ -92,7 +90,9 @@ Decrements the count for `x` by `v` (defaulting to one) | |||
| dec!(ct::Accumulator, x, v::Number) = (ct[x] -= v) | |||
| dec!(ct::Accumulator{T,V}, x) where {T,V} = dec!(ct, x, one(V)) | |||
|
|
|||
| #TODO: once we are done deprecating `pop!` for `reset!` then add `pop!` as an alias for `dec!` | |||
| Base.pop!(ct::Accumulator, x) = dec!(ct, x) | |||
| Base.pop!(ct::Accumulator, x, default) = haskey(ct, x) ? dec!(ct, x) : default | |||
There was a problem hiding this comment.
similarly to what I mentioned above, here I would have expected checking for "value equal zero", not only "has key"
|
In #479 (comment) there is an admonition to not use |
checks off accumulator.jl on #479
One thing I am not sure about is the names
nlargestandnsmallestthose could be changed