Skip to content
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

Fix findmin/findmax to return cartesian indices for BitMatrix #30102

Merged
merged 2 commits into from Nov 28, 2018

Conversation

5 participants
@nalimilan
Copy link
Contributor

commented Nov 20, 2018

For consistency with other AbstractArray types. 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 for Array. What's more, there is currently no equivalent of these optimized methods taking CartesianIndex objects, meaning we use the slower generic fallback. That's lower priority, but I'll file an issue.

@mbauman

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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)

This comment has been minimized.

Copy link
@mbauman

mbauman Nov 20, 2018

Member

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.

This comment has been minimized.

Copy link
@nalimilan

nalimilan Nov 21, 2018

Author Contributor

Indeed. But that will require fixing findnext and findprev to accept cartesian indices (as I noted in the description).

@timholy
Copy link
Member

left a comment

Good catch. I do think this is too big for 1.0.x but obviously this is a critical fix for 1.1.0.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

macOS CI seems to be failing, anyone know why? @staticfloat?

@staticfloat

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

It looks like Travis has changed their image to no longer include ccache. We need to either disable our use of that or get the Travis image to install it again.

EDIT: #30111

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

F&#$ing Travis.

nalimilan added some commits Nov 20, 2018

Move findmin and findmax BitArray methods from LinearAlgebra to Base
There is no reason to have them in LinearAlgebra.
Fix findmin/findmax to return cartesian indices for BitMatrix
For consistency with other AbstractArray types.
The call to keys is optimized out by the compiler for vectors thanks to
@inbounds.

@nalimilan nalimilan force-pushed the nl/findminmax branch from 3af4c68 to a9161d9 Nov 28, 2018

@nalimilan nalimilan merged commit 402c747 into master Nov 28, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@nalimilan nalimilan deleted the nl/findminmax branch Nov 28, 2018

@StefanKarpinski StefanKarpinski added needs news and removed triage labels Dec 6, 2018

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@mbauman or @nalimilan, could you add NEWS for this minor change?

@mbauman

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

NEWS is in add1246#diff-8312ad0561ef661716b48d09478362f3R27 — a similar change that also should get into 1.1.

@mbauman mbauman removed the needs news label Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.