Fix findmin/findmax to return cartesian indices for BitMatrix #30102
Conversation
|
Dang, that's unfortunate. I approve of the idea, but we should try to figure out if this is too big for a minor change. |
| m, mi = false, 1 | ||
| ti = 1 | ||
| ac = a.chunks | ||
| for i = 1:length(ac) |
There was a problem hiding this comment.
Ideally we could just compose with findfirst(B) (and findfirst(!, B) below), but I think the current implementations would be slower than what you have here. Perhaps another place for separate improvements.
There was a problem hiding this comment.
Indeed. But that will require fixing findnext and findprev to accept cartesian indices (as I noted in the description).
|
macOS CI seems to be failing, anyone know why? @staticfloat? |
|
It looks like Travis has changed their image to no longer include EDIT: #30111 |
|
F&#$ing Travis. |
There is no reason to have them in LinearAlgebra.
For consistency with other AbstractArray types. The call to keys is optimized out by the compiler for vectors thanks to @inbounds.
3af4c68
to
a9161d9
Compare
|
@mbauman or @nalimilan, could you add NEWS for this minor change? |
|
NEWS is in add1246#diff-8312ad0561ef661716b48d09478362f3R27 — a similar change that also should get into 1.1. |
For consistency with other
AbstractArraytypes. It's annoying we haven't spotted this earlier, as fixing this inconsistency is technically breaking.The call to keys is optimized out by the compiler for vectors thanks to
@inbounds.BTW, I've also noticed that the
findnext/prev(testf::Function, B::BitArray, start::Integer)methods (just above the diff) accept linear indices even for non-vectors, which is not the case forArray. What's more, there is currently no equivalent of these optimized methods takingCartesianIndexobjects, meaning we use the slower generic fallback. That's lower priority, but I'll file an issue.