-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 count(f,itr) count the number of nonzero; countnz(itr) -> count(itr) #20405
Conversation
base/reduce.jl
Outdated
countnz(a) = count(x -> x != 0, a) | ||
function count end | ||
|
||
function count{F}(pred::F, itr) |
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 static parameter shouldn't be necessary I would think
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 used to be a necessary hint in order to tell the compiler to type-specialize this on function arguments, I thought. (We want to make sure it does this, in order to inline identity
etc.) Has that changed?
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 thought so but I'm probably wrong. @vtjnash?
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.
Hmm, I just tried it, and it seems to inline fine without the parameters now.
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.
IIRC, calling a function argument within the method body triggers specialization on the function argument (hence forcing specialization on pred
via a static parameter is unnecessary in this case). Ref. #19137. Best!
|
||
Count the number of elements in `itr` for which predicate `p` returns `true`. | ||
Count the number of elements in `itr` for which the function `p` returns |
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.
Would make sense to update find
to use iszero
too in a subsequent commit.
""" | ||
count(p, itr) -> Integer | ||
count(itr) |
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.
Repeat -> Integer
? BTW, should be written as ::Integer
now.
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 don't think most of the Base docstrings have made the ::
switch yet. IMO better to wait and do them all at once, keeping ->
in the interim.
|
||
function count(pred, itr) | ||
n = 0 | ||
for x in itr |
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.
Not really related, but should we have a more efficient version for AbstractArray
which would add @inbounds
? When pred=identity
in particular, a SIMD version should be much more efficient.
Closing in favor of #20421 since the |
Closes #20403, #20404.