Implement efficent methods for `any`/`all` by goretkin · Pull Request #32 · JuliaArrays/PaddedViews.jl

goretkin · 2020-04-21T21:30:05Z

Relates to issue #31

johnnychen94 · 2020-04-21T21:50:49Z

Nice observation! I'm too busy to dig into the implementation of #31, if you do I'd be very happy to review it.

timholy · 2020-04-22T20:23:21Z

Would be nice to handle the predicate variants too.

goretkin · 2020-04-22T23:44:33Z

Turns out the first behavior was broken anyway, because it did not properly handle missing. What's pushed now is slower, but should be correct. There should be a way to do short-circuit evaluation with trivalent logic: JuliaLang/julia#35563

One test will be broken, which is due to #33 as far as I can tell.

Relates to issue JuliaArrays#31

One test is failing, due to JuliaArrays#33 Handing `missing` at the cost of doing short-circuit evaluation, see JuliaLang/julia#35563

goretkin · 2020-04-23T01:24:05Z

I rebased to pull in the change in #34

johnnychen94 · 2020-04-23T01:50:18Z

I've tested locally that this patch fixes julia 1.0 and 0.7 tests

if v"0.7" <= VERSION < v"1.1"
    # ambiguity patch
    # https://github.com/JuliaLang/julia/pull/30904
    Base.all(::typeof(isinteger), ::PaddedView{<:Integer}) = true
end

goretkin · 2020-04-23T02:00:14Z

Thanks for sorting that out! Feel free to push that fix, or let me know if I should.

johnnychen94 · 2020-04-23T02:01:39Z

Looks like I don't have push permission to your branch so I'll leave it to you.

goretkin · 2020-04-23T02:09:04Z

Hm, yeah, I'm not sure how that works, since the branch is in my forked repo, but for what it's worth:

johnnychen94 · 2020-04-23T02:12:16Z

Hmmm, no idea how to do that in my local machine. Just did it via github web.

Oh looks like vscode checkouts to another branch..

goretkin · 2020-04-23T02:14:05Z

I don't think I know how to do it either. It would have to be related to this remote, not the goretkin one, so maybe it's some github hack.

Anyway, thanks for the push! Looks like it worked.

johnnychen94 · 2020-04-23T02:16:45Z

Do you have plans to work on #31? Otherwise, we could merge this PR and probably tag a new release.

I'll merge it when you think it's ready.

Would be nice to handle the predicate variants too.

@timholy May I ask what "predicate variants" are? Sorry about this but I've never heard this word. Is that all(::PaddedViews)?

goretkin · 2020-04-23T02:35:41Z

May I ask what "predicate variants" are?

I originally implemented e.g. all(::PaddedView), but now I implemented the more generic varaint which accepts a predicate f : all(f::Function, ::PaddedView). So the PR as it stands should address @timholy's comment.

I'd say this is a safe improvement over the current functionality, so it's good to merge. It's too bad it doesn't have the short-circuiting for efficiency, but that can be added later.

johnnychen94

I was about to merge and then I realized that this PR isn't ready.

🤔 Supporting dims efficiently isn't that trivial a task.

johnnychen94 · 2020-04-23T03:08:52Z

src/PaddedViews.jl

+# The fallbacks work, but this is more efficient
+# TODO use use short-circuit evaluation,
+Base.any(f::Function, A::PaddedView) = f(A.fillvalue) | any(f, parent(A))
+Base.all(f::Function, A::PaddedView) = f(A.fillvalue) & all(f, parent(A))


it doesn't support dims keyword

julia> pa = PaddedView(false, a, (1:2, 1:2)); julia> any(pa; dims=1) ERROR: ArgumentError: No method is implemented for reducing index range of type typeof(i). Please implement reduced_index for this index type or report this as an issue.

johnnychen94 · 2020-04-23T03:09:59Z

src/PaddedViews.jl

+# The fallbacks work, but this is more efficient
+# TODO use use short-circuit evaluation,
+Base.any(f::Function, A::PaddedView) = f(A.fillvalue) | any(f, parent(A))
+Base.all(f::Function, A::PaddedView) = f(A.fillvalue) & all(f, parent(A))


Base.all(A::PaddedView) = all(identity, A)

is still needed, otherwise it calls the fallback.

johnnychen94 · 2020-04-23T03:48:57Z

src/PaddedViews.jl

+# The fallbacks work, but this is more efficient
+# TODO use use short-circuit evaluation,
+Base.any(f::Function, A::PaddedView) = f(A.fillvalue) | any(f, parent(A))
+Base.all(f::Function, A::PaddedView) = f(A.fillvalue) & all(f, parent(A))


julia> a = fill(true, 2, 2) 2×2 Array{Bool,2}: 1 1 1 1 julia> A = PaddedView(false, a, axes(a)) 2×2 PaddedView(false, ::Array{Bool,2}, (Base.OneTo(2), Base.OneTo(2))) with eltype Bool: 1 1 1 1 julia> all(identity, A) false julia> all(identity, collect(A)) true

@goretkin, I pointed this problem in JuliaLang/julia#35563 (comment).

Similarly, there is a problem with cropping.
I have a sense of foreboding about the combination of those and dims.:fearful:

@kimikage Yeah, I am not sure it is worthwhile to implement anything clever for dims. The required reasoning would be substantially more complex, and I can imagine it being slower for many cases where there isn't that much padding.

Not sure what problem with cropping you mean.

julia> a = Bool[0 0; 1 1] 2×2 Array{Bool,2}: 0 0 1 1 julia> A = PaddedView(false, a, (1,3)) 1×3 PaddedView(false, ::Array{Bool,2}, (Base.OneTo(1), Base.OneTo(3))) with eltype Bool: 0 0 0

Thanks for the example! Yeah, that would also need to be handled for sure.

goretkin · 2020-04-27T13:25:35Z

@johnnychen94 Thanks for catching #32 (comment)!

If we want to go forward with this idea, I can implement a [predicate] function that determines whether the fillvalue is activate (i.e. if there's any padding), and use that.

On second thought, a function that counts the number of fillvalue elements would be more generally useful if we wanted a similar optimization for other reductions.

johnnychen94 approved these changes Apr 21, 2020

View reviewed changes

johnnychen94 closed this Apr 23, 2020

johnnychen94 reopened this Apr 23, 2020

goretkin added 2 commits April 22, 2020 21:23

Implement efficent methods for any/all

acfe9dd

Relates to issue JuliaArrays#31

Handle predicate variant of any/all and missing

fadf55b

One test is failing, due to JuliaArrays#33 Handing `missing` at the cost of doing short-circuit evaluation, see JuliaLang/julia#35563

goretkin force-pushed the bool-reductions branch from 6fe1533 to fadf55b Compare April 23, 2020 01:23

julia 0.7/1.0 ambiguity patch for all

cb77668

johnnychen94 requested changes Apr 23, 2020

View reviewed changes

johnnychen94 reviewed Apr 23, 2020

View reviewed changes

Comments

Conversation

goretkin commented Apr 21, 2020

Uh oh!

johnnychen94 commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timholy commented Apr 22, 2020

Uh oh!

goretkin commented Apr 22, 2020

Uh oh!

goretkin commented Apr 23, 2020

Uh oh!

johnnychen94 commented Apr 23, 2020

Uh oh!

goretkin commented Apr 23, 2020

Uh oh!

johnnychen94 commented Apr 23, 2020

Uh oh!

goretkin commented Apr 23, 2020

Uh oh!

johnnychen94 commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goretkin commented Apr 23, 2020

Uh oh!

johnnychen94 commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goretkin commented Apr 23, 2020

Uh oh!

johnnychen94 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goretkin commented Apr 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnnychen94 commented Apr 21, 2020 •

edited

Loading

johnnychen94 commented Apr 23, 2020 •

edited

Loading

johnnychen94 commented Apr 23, 2020 •

edited

Loading

johnnychen94 left a comment •

edited

Loading