JuliaArrays / PaddedViews.jl

Add virtual padding to the edges of an array
Other
49 stars 9 forks source link

Implement efficent methods for `any`/`all` #32

Open goretkin opened 4 years ago

goretkin commented 4 years ago

Relates to issue #31

johnnychen94 commented 4 years ago

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 commented 4 years ago

Would be nice to handle the predicate variants too.

goretkin commented 4 years ago

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: https://github.com/JuliaLang/julia/issues/35563

One test will be broken, which is due to https://github.com/JuliaArrays/PaddedViews.jl/issues/33 as far as I can tell.

goretkin commented 4 years ago

I rebased to pull in the change in https://github.com/JuliaArrays/PaddedViews.jl/pull/34

johnnychen94 commented 4 years ago

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 commented 4 years ago

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

johnnychen94 commented 4 years ago

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

goretkin commented 4 years ago

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

image
johnnychen94 commented 4 years ago

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..

image

goretkin commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

goretkin commented 4 years ago

@johnnychen94 Thanks for catching https://github.com/JuliaArrays/PaddedViews.jl/pull/32#discussion_r413487843!

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.