Skip to content
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

extend reduce_empty to more operators and types #48926

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

Conversation

matthias314
Copy link
Contributor

One can reasonably define an empty reduction reduce(op, T[]) whenever there is a neutral element for the operator op with arguments in T. This PR adds such cases that are currently not defined. Maybe you like some or all of them.

  • At present & and | are only defined for Bool, and xor not at all. I extend these operators to all Integer types, including BigInt. The definition -1 % T I have chosen is equivalent to ~zero(T) for all existing types, but makes fewer allocations for BigInt.

  • I also define the neutral elements for min and max to be typemax(T) and typemin(T). (This works even for the min or max with NaN.) I also extend extrema to work with empty lists.

I can see two concerns one could raise. (Maybe there are others.)

  • In some cases, the neutral elements do not behave well with respect to conversions to larger types. For example, while for T <: Signed one consistently gets reduce(&, T[]) == -1, the result for an unsigned bit integer type T is typemax(T). However, here one might argue that one already has the same problem with arithmetic operations in case of overflow.

  • @timholy made changes to reduce_empty in f2dcc44 to avoid method invalidations. I don't know how my PR affects this. If it is a problem, I'm hoping that one can avoid it by defining reduce_empty for min and max only for certain types, for example T <: Real. (There are other types that define typemin and/or typemax, for example Char, Date and String.)

@matthias314
Copy link
Contributor Author

This seems to good to me now, except that related tests fail in the packages SparseArrays and Statistics because they now get return values instead of the expect error messages. I think that I cannot modify these tests with this PR.

Also note that I have added the default error message to reduce_empty for operators &, |, xor, min and max and type Union{}. However, I wonder how helpful these definitions are. Union{} is the element type of the empty tuple. I guess it's more likely that users instead use an empty list [] with element type Any. For +, *, min and max this leads to a normal method error instead of the dedicated error message. So why bother about Union{}?

@N5N3
Copy link
Member

N5N3 commented Mar 10, 2023

see also #29919, #25595

@matthias314
Copy link
Contributor Author

matthias314 commented Mar 11, 2023

Thanks for the references -- I somehow missed them. Still, I want to point out the following:

The existing definition

mapreduce_empty(::typeof(abs), op, T) = abs(reduce_empty(op, T))

together with this PR (or with #25595) would result in

julia> mapreduce(abs, &, Int[])
1

which is not meaningful. I would argue that this is not a deficiency of this PR, but of the method definition displayed above: When computing mapreduce(f, op, iter), the function f is applied to the elements of iter first, and then the operator op is applied. There are cases like

mapreduce_empty(f::typeof(abs), ::typeof(max), T) = abs(zero(T))

where one can argue that f effectively restricts the domain on which op is defined, and this is reflected by applying f to the neutral element of op. In general, however, is doesn't make sense to apply f to the neutral element. (In the example above, the neutral element is -1.)

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label Sep 3, 2024
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.

3 participants