add count(itr) and throw and error in count if non-boolean values are encountered #20421
Conversation
…f non-boolean values are encountered (fixes JuliaLang#20404)
| function count(pred, a::AbstractArray) | ||
| n = 0 | ||
| for i in eachindex(a) | ||
| @inbounds n += pred(a[i])::Bool |
There was a problem hiding this comment.
Is this safe for arbitrary preds and AbstractArrays? Could, for example, pred resize a::Vector as a side effect, causing issues downstream in the iteration? Best!
There was a problem hiding this comment.
We seem to use @inbounds for all of the other mapreduce functions, so they are all assuming that the mapped function does not resize the array. Maybe we should reconsider that elsewhere, but I don't think that count should be the exception here.
There was a problem hiding this comment.
Hm, interesting. On the one hand, from base/reduce.jl _mapreduce, mapreduce_impl, mapfoldl_impl, and sum_kbn use @inbounds. On the other hand, mapfoldr_impl, any, all, contains, extrema, and the existing implementation of count do not.
There was a problem hiding this comment.
Sounds like a reasonable assumption to me. Better turn on @inbounds everywhere possible.
There was a problem hiding this comment.
Ref. related discussion #19925 (review). Perhaps consistency of @inbounds decoration in reductions and similar functions deserves a dedicated issue? Best!
There was a problem hiding this comment.
@JeffBezanson seemed to imply that @inbounds was only OK for Array, for which we know the index is valid. Indeed, for a custom AbstractArray, a buggy implementation could lead to crashes with @inbounds. We should definitely have a policy about this.
There was a problem hiding this comment.
We have a policy: only use @inbounds when you can be certain, from local information, that all accesses are in bounds.
There was a problem hiding this comment.
So that means the signature needs to be changed to Array? We cannot be certain that eachindex(a) is correct for any custom type.
There was a problem hiding this comment.
Maybe another PR should go through and fix occurrences of @inbounds in Base for AbstractArray?
It would be nice to have a way to turn this on for Array without requiring two methods. See also #15291
There was a problem hiding this comment.
I've opened #20469 so that we don't forget about this.
|
Should be good to merge. |
5 participants
Fixes #20403 and #20404. Since changing
countnzwas controversial, I left that function alone.