Skip to content

counting and summing Bools with a small integer init promote to Int #58374

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 5 commits into from
May 22, 2025

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented May 10, 2025

This is a small but concrete and independent change that can easily be split out from #58241 and is required for more pairwise reassociations (like #52397).

Specifically, it's required because count and sum use Base.add_sum, which very intentionally promotes small integers to Int or Uint. And if you add two Bools together, they also promote to Int. But if an init=0x00 is provided, then we get into a strange situation. This is the status quo on master right now:

julia> const += Base.add_sum
add_sum (generic function with 4 methods)

julia> (((0x00 +true) +true) +true) +true
0x04

julia> ((0x00 +true) +true) +ₛ ((0x00 +true) +true)
0x0000000000000004

In other words, if we happen to choose to re-associate the branches here, we end up with a 0x02 +ₛ 0x02, which promotes to Uint.

This change here adds Bool into the extra promotion behaviors for add_sum. So with this commit, the above session becomes:

julia> (((0x00 +true) +true) +true) +true
0x0000000000000004

julia> ((0x00 +true) +true) +ₛ ((0x00 +true) +true)
0x0000000000000004

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label May 10, 2025
@JuliaLang JuliaLang deleted a comment May 10, 2025
@JuliaLang JuliaLang deleted a comment May 10, 2025
@JuliaLang JuliaLang deleted a comment May 10, 2025
@JuliaLang JuliaLang deleted a comment May 10, 2025
@ViralBShah ViralBShah requested a review from Copilot May 10, 2025 16:04
Copilot

This comment was marked as resolved.

for accumulate inference
@mbauman
Copy link
Member Author

mbauman commented May 13, 2025

OK, so upon reflection here, I think we should just always promote all small integers to Int. In fact, that's what this PR branch Julia (on both current master and this branch!) would do if the mapreduce machinery consistently called reduce_first.

@mbauman mbauman marked this pull request as draft May 13, 2025 21:42
@mbauman mbauman changed the title counting and summing Bools with a small integer init promote to [U]Int counting and summing Bools with a small integer init promote to Int May 14, 2025
@mbauman
Copy link
Member Author

mbauman commented May 14, 2025

@nanosoldier runtests()

@mbauman mbauman marked this pull request as ready for review May 14, 2025 14:13
@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

14 packages failed only on the current version.

  • Package has test failures: 4 packages
  • Package tests unexpectedly errored: 2 packages
  • Tests became inactive: 1 packages
  • Test duration exceeded the time limit: 5 packages
  • Test log exceeded the size limit: 2 packages

1240 packages failed on the previous version too.

✔ Packages that passed tests

25 packages passed tests only on the current version.

  • Other: 25 packages

5370 packages passed tests on the previous version too.

~ Packages that at least loaded

11 packages successfully loaded only on the current version.

  • Other: 11 packages

2975 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

920 packages were skipped on the previous version too.

@adienes
Copy link
Member

adienes commented May 22, 2025

I think Bool seems more Signed-y than Unsigned-y, going off the fact that sign(x::T) == one(T) for all x isa Unsigned, but sign(false) != one(Bool)

so the promotion proposed here would fall out of that and makes sense to me

@LilithHafner
Copy link
Member

Triage thinks this is clearly a good idea.

@LilithHafner LilithHafner merged commit 3f1fc0d into JuliaLang:master May 22, 2025
8 checks passed
@LilithHafner
Copy link
Member

Should we do this with mul_prod, too?

@mbauman
Copy link
Member Author

mbauman commented Jun 30, 2025

Should we do this with mul_prod, too?

That's a very good question. I had initially dismissed it without much thought because, well, multiplying by booleans doesn't do much. In fact, there's no need for a widening at all, because the booleans multiply with other types as either their identity or zero. And if we're considering an init, that's gotta be a one of some sort.

But we still have the same (potential) type instability when combining multiple intermediate results as above. prod(ones(10), init=0x1)) won't have the same problem with the returned value, but the exact type is unstable based upon reduction strategy. So yes, seems quite prudent to match the definitions here.

@mbauman mbauman deleted the mb/make-bools-count branch June 30, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fold sum, maximum, reduce, foldl, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants