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

Filter(f, tuple) #32968

Merged
merged 8 commits into from
Dec 3, 2019
Merged

Filter(f, tuple) #32968

merged 8 commits into from
Dec 3, 2019

Conversation

mcabbott
Copy link
Contributor

Closes #30418

@mcabbott
Copy link
Contributor Author

Test failure should be fixed by #32965?

To be able to filter by type, it's tempting to add something like this, working around the lack of isa(T), #32018, but probably too weird:

@generated function _filter(T::DataType, x::Tuple, r::Tuple{})
    out = []
    for (d,t) in enumerate(x.parameters)
        if t isa T
            push!(out, :( x[$d] ))
        end
    end
    :( ($(out...),) )
end

@StefanKarpinski
Copy link
Member

Yes, I can confirm that the doctest failure was my fault and unrelated to this :sheepish_grin:

@tkf
Copy link
Member

tkf commented Aug 20, 2019

I'd suggest something simpler:

filter(f, t::Tuple) = _filterargs(f, t...)
_filterargs(f) = ()
_filterargs(f, x, xs...) =
    f(x) ? (x, _filterargs(f, xs...)...) : _filterargs(f, xs...)

It can handle isa in lambda:

julia> f(::Type{T}) where T = filter(x -> x isa T, (0, 1.0, 2, 3.0))
f (generic function with 1 method)

julia> @code_typed f(Int)
CodeInfo(
1%1 = Core.tuple(0, 2)::Tuple{Int64,Int64}
└──      return %1
) => Tuple{Int64,Int64}

@mcabbott
Copy link
Contributor Author

I played with a few variants, this one is fast on the things I was testing:

@btime filter(x->x>2, (1,2,3,4,5)) # 19.337 ns this PR, 527.370 ns tkf's, 1.250 μs my most compact

But @tkf's is faster on x -> x isa Int

@btime filter(x -> x isa Int, (0,true,1,2.0,π,4)) # 17.555 ns this PR, 1.696 ns tkf's
@btime filter(Int, (0,true,1,2.0,π,4)) # 0.029 ns with @generated

But I don't know exactly what is fair in timing such things, @btime () -> f(Int) with the above f gives 0.029 ns for both variants.

@mcabbott
Copy link
Contributor Author

Polite bump — what can I do to push this along?

@StefanKarpinski
Copy link
Member

@JeffBezanson, please merge if this looks acceptable to you.

@tkf
Copy link
Member

tkf commented Sep 25, 2019

Actually, here is an even simpler and better approach equivalent to #32968 (comment) (for short tuples):

filter(f, xs::Tuple) = afoldl((ys, x) -> f(x) ? (ys..., x) : ys, (), xs...)

@mcabbott IMHO, I think you need to re-write your code. For example, filter(x -> x !== nothing, (1, missing, 2)) cannot be inferred with your version but my versions can. Generally, I think it's a better approach to not use tail-recursion on fixed-length data structures like tuple (see, e.g., foldl and map for tuples). IIUC, this is because the compiler does not like recursion when the arguments are not simplifying.

@mcabbott
Copy link
Contributor Author

Thanks, that’s very neat, I did not know about afoldl. That now seems better & faster than mine in all respects. So I will replace the PR with two lines once I’ve checked a bit more… tests so far are here:
https://gist.github.com/mcabbott/1a6db9914ec6b941db6d2218b65de000

@mcabbott
Copy link
Contributor Author

Done! I also added a news item.

@mcabbott
Copy link
Contributor Author

Bump: @JeffBezanson can you review when you have a minute?

@mcabbott
Copy link
Contributor Author

Bump?

Red X is from win32, "cp: cannot stat 'dist-extras/7z.*’”, surely unrelated.

@StefanKarpinski StefanKarpinski merged commit 88dfa2e into JuliaLang:master Dec 3, 2019
@mcabbott mcabbott deleted the filter branch December 3, 2019 19:53
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
Co-authored-by: Takafumi Arakaki <[email protected]>
@N5N3 N5N3 mentioned this pull request Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filter(::Function, ::Tuple) is not defined
3 participants