Skip to content

Dimensional any and all reduce with Boolean operations #58185

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

Merged
merged 7 commits into from
Apr 29, 2025

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Apr 21, 2025

and not bitwise ones. Specifically, they are now reductions using these operators:

and_all(x, y) = (x && y)::Bool
or_any(x, y) = (x || y)::Bool
# As a performance optimization, avoid runtime branches:
and_all(x::Bool, y::Bool) = (x & y)::Bool
or_any(x::Bool, y::Bool) = (x | y)::Bool

Fixes half of #45562.

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label Apr 21, 2025
@mbauman mbauman force-pushed the mb/not-anything-goes branch from 9f04f17 to fd531e4 Compare April 21, 2025 19:40
@mbauman
Copy link
Member Author

mbauman commented Apr 21, 2025

I split this out from #55628 as it seems slightly more clearly like a bugfix — but do note that it required some changes to doctests! Specifically, any!([0 0], [true true]) is now an error. So I still don't think that it should necessarily be backported.

@mbauman mbauman added the bugfix This change fixes an existing bug label Apr 21, 2025
@mbauman mbauman requested a review from LilithHafner April 21, 2025 20:14
Copy link
Member Author

@mbauman mbauman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preserve and test more of the existing behaviors

@testset "issue #45562" begin
@test all([true, true, true], dims = 1) == [true]
@test any([true, true, true], dims = 1) == [true]
@test_throws TypeError all([3, 3, 3], dims = 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line the only intended behavior change of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, along with the corresponding change to any(; dims) and any! and all!.

and whitespace in tests

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
@LilithHafner
Copy link
Member

Seems like a good idea to require the arguments to all and any to be Bool arrays (or reductions producing Bools). This makes cases that were previously three quarters broken into uniform errors so @nanosoldier runtests(). Nevertheless the bugfix label seems much more accurate than minor change here.

Does this impact performance?

@mbauman
Copy link
Member Author

mbauman commented Apr 22, 2025

Just to detangle all the reductions work here:

  • This fix is required to make empty dimensional reductions behave the same as their whole-array counterparts because that PR adds a branch:
    isempty(A) && return fill(mapreduce_empty(f, op, eltype(A)), reduced_indices(A, dims))
    In the case of any and all, that mapreduce_empty needs to return false and true, respectively. That isn't the case when these reductions are implemented using bitwise operators. So that's why this fix is currently incorporated into unify reduce/reducedim empty case behaviors #55628 (where nanosoldier is also currently running).
  • This does not fix the problem noted in Bug in function all #45562 where dimensional reductions with bitwise operators get the result array type wrong. Fix that is much more enmeshed with getting initialization correct in the first place — and that's the big WIP: a more principled take on dimensional reduction inits #55318 refactor.
  • My overall strategy is to split out the behavior changes like these when they can be easily implemented independently of (and in advance of) that big refactor.

@mbauman
Copy link
Member Author

mbauman commented Apr 22, 2025

Does this impact performance?

Yeah, looks like this inhibits SIMD. On this branch right now:

julia> @btime any(x; dims=1) setup=(x=falses(10000,10))
  126.083 μs (3 allocations: 112 bytes)
1×10 BitMatrix:
 0  0  0  0  0  0  0  0  0  0

julia> @eval Base.or_any(x,y) = Bool(Bool(x) | Bool(y))

julia> @btime any(x; dims=1) setup=(x=falses(10000,10))
  37.084 μs (3 allocations: 112 bytes)
1×10 BitMatrix:
 0  0  0  0  0  0  0  0  0  0

julia> @eval Base.or_any(x::Bool,y::Bool) = (x | y)::Bool

julia> @btime any(x; dims=1) setup=(x=falses(10000,10))
  27.458 μs (3 allocations: 112 bytes)
1×10 BitMatrix:
 0  0  0  0  0  0  0  0  0  0

The last matches nightly.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

2 packages crashed on the previous version too.

✖ Packages that failed

29 packages failed only on the current version.

  • Illegal method overwrites during precompilation: 1 packages
  • Package has test failures: 5 packages
  • Package tests unexpectedly errored: 1 packages
  • Tests became inactive: 1 packages
  • Test duration exceeded the time limit: 21 packages

1418 packages failed on the previous version too.

✔ Packages that passed tests

24 packages passed tests only on the current version.

  • Other: 24 packages

5259 packages passed tests on the previous version too.

~ Packages that at least loaded

1 packages successfully loaded only on the current version.

  • Other: 1 packages

2855 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

916 packages were skipped on the previous version too.

@mbauman
Copy link
Member Author

mbauman commented Apr 24, 2025

Great, I think this is looking good. All the failures look to be numeric instabilities or timeouts in packages that tend to have numeric instabilities or timeouts... and this should manifest as a very clear new TypeError in places that previously worked (but were buggy) — I don't see that in any of the logs.

@mbauman mbauman merged commit 208560b into JuliaLang:master Apr 29, 2025
8 checks passed
@mbauman mbauman deleted the mb/not-anything-goes branch April 29, 2025 15:00
charleskawczynski pushed a commit to charleskawczynski/julia that referenced this pull request May 12, 2025
…#58185)

and not bitwise ones. Specifically, they are now reductions using these
operators:

```julia
and_all(x, y) = (x && y)::Bool
or_any(x, y) = (x || y)::Bool
# As a performance optimization, avoid runtime branches:
and_all(x::Bool, y::Bool) = (x & y)::Bool
or_any(x::Bool, y::Bool) = (x | y)::Bool
```

Fixes half of JuliaLang#45562.

---------

Co-authored-by: Lilith Hafner <Lilith.Hafner@gmail.com>
Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug fold sum, maximum, reduce, foldl, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants