PaddedViews.jl icon indicating copy to clipboard operation
PaddedViews.jl copied to clipboard

Implement efficent methods for `any`/`all`

Open goretkin opened this issue 4 years ago • 13 comments

Relates to issue #31

goretkin avatar Apr 21 '20 21:04 goretkin

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

johnnychen94 avatar Apr 21 '20 21:04 johnnychen94

Would be nice to handle the predicate variants too.

timholy avatar Apr 22 '20 20:04 timholy

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 avatar Apr 22 '20 23:04 goretkin

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

goretkin avatar Apr 23 '20 01:04 goretkin

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

johnnychen94 avatar Apr 23 '20 01:04 johnnychen94

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

goretkin avatar Apr 23 '20 02:04 goretkin

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

johnnychen94 avatar Apr 23 '20 02:04 johnnychen94

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

image

goretkin avatar Apr 23 '20 02:04 goretkin

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

johnnychen94 avatar Apr 23 '20 02:04 johnnychen94

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.

goretkin avatar Apr 23 '20 02:04 goretkin

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)?

johnnychen94 avatar Apr 23 '20 02:04 johnnychen94

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 avatar Apr 23 '20 02:04 goretkin

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

goretkin avatar Apr 27 '20 13:04 goretkin