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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

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.

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.

4 participants