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

add more methods and tests for reductions over empty arrays #29919

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

simonbyrne
Copy link
Contributor

From discussion in #28535.

@ararslan ararslan added arrays [a, r, r, a, y, s] maths Mathematical functions labels Nov 3, 2018
base/reduce.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Apr 9, 2021
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Can you update this so that the doctest passes?

@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Apr 9, 2021
@StefanKarpinski
Copy link
Member

Bump. Needs doctest fixes. Otherwise good to go.

@timholy
Copy link
Member

timholy commented Aug 18, 2021

Now that we have maximum(itr; init=typemin(eltype(itr))), is this really the way we want to go? What if someone wanted the error? The init approach lets the user/developer choose.

#41885 will print a nice warning suggesting to supply an init value.

@timholy
Copy link
Member

timholy commented Aug 18, 2021

But 👍 to more tests!

@tkf
Copy link
Member

tkf commented Aug 18, 2021

Is it OK to get false from -1 <= maximum(sin.(xs)) <= 1? (I prefer an error)

@oscardssmith oscardssmith added the triage This should be discussed on a triage call label Nov 17, 2021
@oscardssmith
Copy link
Member

Let's figure out if this is OK to merge and then merge it.

Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

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

FWIW, I'm opposed to this change. We don't know the domain on which the user data is defined and we don't know if typemin and typemax are meaningful values (see an example above). I think nudging users to specify the identity element via init will help them write more robust programs.

That said, I think the following tweaks might make sense if we go with this.

base/reduce.jl Outdated Show resolved Hide resolved
base/reduce.jl Outdated Show resolved Hide resolved
base/reduce.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

Triage thinks it's ok to add these, with tkf's suggestions. maximum(sin.(xs)) where xs is empty is unfortunate, but all you're doing is passing an empty array to maximum, so it doesn't matter how you got it. maximum(sin, xs) is different, and should remain an error.

@JeffBezanson JeffBezanson added forget me not PRs that one wants to make sure aren't forgotten and removed triage This should be discussed on a triage call forget me not PRs that one wants to make sure aren't forgotten labels Nov 18, 2021
simonbyrne and others added 4 commits November 23, 2021 13:50
Co-authored-by: Takafumi Arakaki <aka.tkf@gmail.com>
Co-authored-by: Takafumi Arakaki <aka.tkf@gmail.com>
Co-authored-by: Takafumi Arakaki <aka.tkf@gmail.com>
@vtjnash vtjnash merged commit 09617ac into master Nov 2, 2023
@vtjnash vtjnash deleted the sb/reduce-empty branch November 2, 2023 19:31
vtjnash added a commit that referenced this pull request Nov 2, 2023
vtjnash added a commit that referenced this pull request Nov 2, 2023
…52003)

Reverts #29919

CI was older than I realized on this, so this needed some updates to
tests and docstrings
vtjnash added a commit that referenced this pull request Apr 16, 2024
@DilumAluthge DilumAluthge removed the forget me not PRs that one wants to make sure aren't forgotten label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants