Updated `filter!` to take an `AbstractVector` vs a `Vector`. #21417

rofinn · 2017-04-17T17:34:09Z

Allow filter! to operate on AbstractVectors. This pushes the requirement to whether or not deleteat! is implemented for the array. My use case is that I want filter! to work on NullableArrays and DataArrays, which already have deleteat! implemented.

nalimilan · 2017-04-17T20:10:41Z

base/array.jl

@@ -1805,7 +1805,7 @@ julia> filter(isodd, a)
 """
 filter(f, As::AbstractArray) = As[map(f, As)::AbstractArray{Bool}]

-function filter!(f, a::Vector)
+function filter!(f, a::AbstractVector)
 insrt = 1


This will only work for 1-based linear indexing arrays. Would be better to iterate over eachindex(a).

Alright, I've made that change and added some tests to make sure it works. Is there an array type in Base that I can test against which doesn't use 1-based linear indexing?

nalimilan · 2017-04-19T07:56:32Z

test/arrayops.jl

+ filter!(x -> x > 5, a)
+
+ # different subtype of AbstractVector
+ @test a == collect(6:10)


Shouldn't this line be in the previous block?

rofinn · 2017-04-20T18:07:04Z

@tkelman and @nalimilan: Are there any other changes you'd like made or any other tests you'd like added before this can be merged?

nalimilan · 2017-04-21T06:58:23Z

base/array.jl

 end
 end
- deleteat!(a, insrt:length(a))
+
+ deleteat!(a, i:last(idx))


I wonder whether it could be more efficient to do resize!(a, len), even though this implies computing len. Not for this PR though.

nalimilan · 2017-04-21T07:03:26Z

base/array.jl

+function filter!(f, a::AbstractVector)
+ idx = eachindex(a)
+ state = start(idx)
+ (i, state) = next(idx, state)


Sorry, I had missed this: you need a done(idx, state) && return a check before this for empty arrays. The tests didn't catch this since the implementation for Array doesn't throw an error (and just returns state+1), but in general it could fail.

BTW, you can remove parentheses on LHS for tuple assignment (here and below).

@nalimilan The next behaviour was kind of intentional here as we want i to be at state+1 passed the end of the new last index when we get to the deleteat!. I suppose a done(idx, state) or isempty(a) check would avoid needing to call deleteat!, but we still run into the issue of going passed the end of the array with the next(idx, state) call in the for loop. This issue would probably be better circumvented with Keno's revised iteration protocol julep.

Since you just need to return immediately when the input is empty, I don't see why deletat! enters the picture at all.

tkelman · 2017-04-21T19:08:35Z

@timholy would probably suggest testing against a non-1-indexed array type from TestHelpers, as well

… `OffsetArray` tests.

rofinn · 2017-04-21T21:59:21Z

@tkelman Thanks, that's exactly what I was looking for.

nalimilan · 2017-04-22T14:38:42Z

base/array.jl

- a[insrt] = acurr
- insrt += 1
+function filter!(f, a::AbstractVector)
+ if !isempty(a)


isempty(a) && return a would be better style IMHO since it avoids wrapping the whole body inside the condition.

I get not wanting to wrap the entire thing in an if, but I generally try to avoid putting multiple return statements in a function.

That's a common style in the Julia code base for simple initial checks like this.

nalimilan · 2017-04-22T14:39:30Z

test/TestHelpers.jl

@@ -198,6 +198,24 @@ end
 val
 end

+@inline function Base.deleteat!(A::OffsetArray, i::Int)
+ checkbounds(A, i)


A @boundscheck annotation would be useful here and below.

Or maybe you don't need this and offset could just do it for you? @timholy?

While not a big deal, changing the bounds checking in the OffsetArray code feels a little outside the scope of this PR. As it stands, this has progressed from a 1 word change to partly rewriting filter!, adding tests and extending the OffsetArrays code.

Actually, my suggestion aimed at requiring less changes, not more. If that involves doing changes elsewhere then I agree it's not the place to do it.

Anyway, just add the @boundscheck annotations and it should be good to merge, though a quick review of the OffsetArray parts by @timholy would be nice.

I still think making a separate PR to put all the checkbounds calls for OffsetArrays inside an @boundscheck would keep things more organized and consistent.

@nalimilan I don't think it makes sense to add @boundscheck annotations in one place but not the other very similar cases in the same file for the same type. I think those changes would be best made all at once in a separate PR.

Fine with me. I hadn't realized the existing functions didn't use this.

nalimilan · 2017-04-22T17:13:08Z

I just realized that the new function relies on the assumption that one can index an AbstractArray with the iteration state. That's fine in this case since this is true for Base array types and because the new method is a strict improvement over the current state, but we should really do something about this. The same issue was raised in #18668 (comment).

…le line check.

iamed2 · 2017-04-23T22:27:47Z

@nalimilan It doesn't make that assumption. It iterates over each index and indexes with those.

omus

I'll merge this tomorrow

JeffBezanson approved these changes Apr 17, 2017

View reviewed changes

nalimilan reviewed Apr 17, 2017

View reviewed changes

tkelman added the needs tests Unit tests are required for this change label Apr 17, 2017

rofinn force-pushed the filter-on-abstract-vector branch from b3e6898 to a6c6e97 Compare April 19, 2017 03:03

nalimilan reviewed Apr 19, 2017

View reviewed changes

rofinn force-pushed the filter-on-abstract-vector branch from b18bea3 to 72dd093 Compare April 19, 2017 15:46

tkelman removed the needs tests Unit tests are required for this change label Apr 20, 2017

nalimilan reviewed Apr 21, 2017

View reviewed changes

rofinn and others added 5 commits April 21, 2017 16:51

Updated filter! to take an AbstractVector vs a Vector.

ba7a265

filter!(f, a::AbstractVector) now uses arbitrary indices

950db94

Added some tests for filter!(f, a::AbstractVector)

d3f22fd

Fixed misplaced comment in filter! tests

4c8d066

Added isempty check to filter!(f, a::AbstractVector) and added some…

ac07ec5

… `OffsetArray` tests.

rofinn force-pushed the filter-on-abstract-vector branch from cfc0674 to ac07ec5 Compare April 21, 2017 21:52

nalimilan reviewed Apr 22, 2017

View reviewed changes

Replaced isempty block in filter!(f, a::AbstractVector) with sing…

9959cc0

…le line check.

nalimilan approved these changes Apr 24, 2017

View reviewed changes

omus approved these changes Apr 25, 2017

View reviewed changes

omus merged commit 2c704d1 into JuliaLang:master Apr 25, 2017

omus deleted the filter-on-abstract-vector branch April 25, 2017 14:35

vtjnash mentioned this pull request May 7, 2019

speed up filter of vector #31929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated `filter!` to take an `AbstractVector` vs a `Vector`. #21417

Updated `filter!` to take an `AbstractVector` vs a `Vector`. #21417

rofinn commented Apr 17, 2017

nalimilan Apr 17, 2017

rofinn Apr 19, 2017 •

edited

Loading

nalimilan Apr 19, 2017

rofinn commented Apr 20, 2017

nalimilan Apr 21, 2017

nalimilan Apr 21, 2017

rofinn Apr 21, 2017 •

edited

Loading

nalimilan Apr 22, 2017

tkelman commented Apr 21, 2017

rofinn commented Apr 21, 2017

nalimilan Apr 22, 2017

rofinn Apr 22, 2017

nalimilan Apr 22, 2017

nalimilan Apr 22, 2017

nalimilan Apr 22, 2017

rofinn Apr 23, 2017

nalimilan Apr 24, 2017

rofinn Apr 24, 2017

iamed2 Apr 24, 2017

nalimilan Apr 24, 2017

nalimilan commented Apr 22, 2017

iamed2 commented Apr 23, 2017

omus left a comment

Updated filter! to take an AbstractVector vs a Vector. #21417

Updated filter! to take an AbstractVector vs a Vector. #21417

Conversation

rofinn commented Apr 17, 2017

Choose a reason for hiding this comment

rofinn Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 21, 2017

rofinn commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Apr 22, 2017

iamed2 commented Apr 23, 2017

omus left a comment

Choose a reason for hiding this comment

Updated `filter!` to take an `AbstractVector` vs a `Vector`. #21417

Updated `filter!` to take an `AbstractVector` vs a `Vector`. #21417

rofinn Apr 19, 2017 •

edited

Loading

rofinn Apr 21, 2017 •

edited

Loading