make counting more robust to input datatype #722
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ length(wv::AbstractWeights) = length(wv.values) | |
| sum(wv::AbstractWeights) = wv.sum | ||
| isempty(wv::AbstractWeights) = isempty(wv.values) | ||
| size(wv::AbstractWeights) = size(wv.values) | ||
| Base.axes(wv::AbstractWeights) = Base.axes(wv.values) | ||
LilithHafner marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Is this definition correct? We only support There was a problem hiding this comment. The As for For example using StatsBase
w = Weights([4,2,6])
w[0x2] = 1
display(w)
#=
3-element Weights{Int64, Int64, Vector{Int64}}:
4
1
6
=#The previous implicit definition ( using StatsBase, OffsetArrays
w = Weights(OffsetVector([4,2,6], 2))
display(w)
#=
3-element Weights{Int64, Int64, OffsetVector{Int64, Vector{Int64}}}:
3 # unsafe read
4972085568 # unsafe read
4
=#On this PR using StatsBase, OffsetArrays
w = Weights(OffsetVector([4,2,6], 2))
display(w)
#=
3-element Weights{Int64, Int64, OffsetVector{Int64, Vector{Int64}}} with indices 3:5:
4
2
6
=#There was a problem hiding this comment. Given the discussion in #791 I would propose to add tests of the consequences of this change (e.g. making sure that There was a problem hiding this comment. This change does not add any new functionality, it simply removes segfaults from existing functionality. @bkamins, would you be willing to create a PR adding those tests? There was a problem hiding this comment. if StatsBase.jl maintainers are OK to merge this PR without these tests then I can add them later. There was a problem hiding this comment. I guess it's better to avoid known segfaults (or worse, incorrect behavior) even if not tested completely, so in the interest of avoiding this PR from being forgotten again we could merge without additional tests. |
||
|
|
||
| Base.convert(::Type{Vector}, wv::AbstractWeights) = convert(Vector, wv.values) | ||
|
|
||
|
|
@@ -299,6 +300,7 @@ sum(wv::UnitWeights{T}) where T = convert(T, length(wv)) | |
| isempty(wv::UnitWeights) = iszero(wv.len) | ||
| length(wv::UnitWeights) = wv.len | ||
| size(wv::UnitWeights) = tuple(length(wv)) | ||
| Base.axes(wv::UnitWeights) = tuple(Base.OneTo(length(wv))) | ||
|
|
||
| Base.convert(::Type{Vector}, wv::UnitWeights{T}) where {T} = ones(T, length(wv)) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
better expresses the intention in the original code. Also maybe this condition should be noted in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the previous version errored when
rwas larger thanlevelsand if we keep that restriction, it should be noted in the documentation.However, it feels more natural to me not to keep that restriction, especially since it is not documented. So long as what we are adding to
rfits intor, this method will work and other entries ofrwill not be affected. This looser restriction and guarantee not to mess with other entries ofris implicit and need not be mentioned in the docstring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is broader. With your code if
risOffsetArraythen indices from1tolength(levels)must be valid in it. But then you are adding values torin indices starting from1, which in general does not have to be first element ofr. The docstring does not specify where the elements are put intor. There can be two options:1orrThat is why I propose to add
firstindex(r)==1condition which resolves this issue as then both options are the same.I agree that we could allow
rto be longer thanlength(levels)(this is a change, but I think it is acceptable and makes sense).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. There could be an ambiguity here. I see two solutions:
Base.require_one_based_indexing(r))I prefer option 1 because I think the choice made here is distinctly more natural than the choice to start at
firstindex(r)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 1 sounds fine, the way
ris used should have been explained better anyway in the current version (one could have imagined that the value of each level was used as to index intorrather than starting at 1). Allowingrto be longer thanlength(levels)sounds OK too as long as it's documented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - it is chcecked. My point is that it should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalmarek
Could you link that issue? I'm not seeing it and want to make sure the documentation I added addresses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it wasn't a gh issue, but in my comments about
addcounts!I missed the fact thatcheckboundsforcesrto accept indices from1tolenght(levels), sincelevels::IntUnitRange. This is something I'd call "too clever to maintain" in the long run, unless explicitly documented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness, here is the link: #722 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This thread is about ambiguity in behavior when passed an
rthat uses offset indexing. That ambiguity is resolved in the current documentation: "For eachxi ∈ x, ifxi == levels[j], then we incrementr[j]". This is a different issue than the one @kalmarek raised in the linked comment which is about whether this method could segfault on certain nonstandard indices.I pushed another commit that should address @kalmarek's latest critique. Even though we know
firstindex(levels) == 1, we can still writefirstindex(levels)instead of1. (This should future proof this method in the event that Julia switches to 2 based indexing at any point in the future :), or, much more plausibly, ifIntUnitRangeever allows offset indices.)