Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make counting more robust to input datatype #722
make counting more robust to input datatype #722
Changes from 6 commits
3905f6e
8eb5855
84ef887
d82295d
a4505b2
c334d4d
94b3e6c
87074b2
aa21bc9
309586f
3b5e01d
3384f45
6b81b51
18b70b5
7aa2d79
93caf72
62c5967
1238a2d
c1c09d1
bc20432
cbfd92e
60c0aad
c72b1aa
e4ca264
00fe408
a470531
aca38a3
168b3ab
204f89a
3f3e657
657c1ec
2ebf1f3
f469f0c
6cf64d9
dc1f7ae
4fd45d9
d31dcce
0fd1c27
fbdf564
e1fab99
717c795
ba59cc3
2dfba9f
8f446ee
e20e28c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this definition correct? We only support
Integer
indices ingetindex
andInt
insetindex!
below it seems, so non-standard axes that don't operate withInteger
s will yield unsupported indices it seems.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
AbstractArray
interface requires thataxes
returns a tuple ofAbstractUnitRange{<:Integer}
s, so we can assume that theireltype
will be anInteger
. From theaxes
docstring: "Each of the indices has to be anAbstractUnitRange{<:Integer}
".As for
Integers
other thanInt
, As long as we implementgetindex
andsetindex
forInt
as required by the AbstractArray interface, Julia's Base libraries will convert otherInteger
s toInt
as needed.For example
The previous implicit definition (
axes(wv::AbstractWeights) = (Base.OneTo(length(wv)),)
) was not correctOn this PR
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.
Given the discussion in #791 I would propose to add tests of the consequences of this change (e.g. making sure that
getindex
,setindex!
,firstindex
,lastindex
,isequal
etc. work correctly for non-1-based case)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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.