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

mean(::UnitRange) overflows #50982

Closed
jariji opened this issue Aug 19, 2023 · 6 comments · Fixed by JuliaStats/Statistics.jl#150
Closed

mean(::UnitRange) overflows #50982

jariji opened this issue Aug 19, 2023 · 6 comments · Fixed by JuliaStats/Statistics.jl#150
Labels
good first issue Indicates a good issue for first-time contributors to Julia kind:bug Indicates an unexpected problem or unintended behavior

Comments

@jariji
Copy link
Contributor

jariji commented Aug 19, 2023

using Statistics
julia> mean(1:typemax(Int64))
-4.611686018427388e18
  mean(itr)

  Compute the mean of all elements in a collection.

From the documentation one might expect a number between the two endpoints, but I got a number outside that range. I think this is a bug.

@oscardssmith
Copy link
Member

Good catch. Right now this is implemented as (first(r) + last(r)) / 2. It should probably be first(r)/2 + last(r)/2.

@oscardssmith oscardssmith added kind:bug Indicates an unexpected problem or unintended behavior good first issue Indicates a good issue for first-time contributors to Julia labels Aug 19, 2023
@petvana
Copy link
Member

petvana commented Aug 20, 2023

Seems to be already reported in JuliaStats/Statistics.jl#120 and there is even an unmerged PR for that JuliaStats/Statistics.jl#115.

@chunjiw
Copy link

chunjiw commented Aug 31, 2023

Good catch. Right now this is implemented as (first(r) + last(r)) / 2. It should probably be first(r)/2 + last(r)/2.

Or perhaps first(r) + (last(r) - first(r)) / 2

@jariji
Copy link
Contributor Author

jariji commented Aug 31, 2023

I like @chunjiw 's proposal because it works better on affine spaces like dates or Fahrenheits where you can't divide positions by numbers (you can only divide displacements).

@chunjiw
Copy link

chunjiw commented Aug 31, 2023

I'm wondering is the performance gain of "one less float point division" significant enough? When I started programming I was taught x + (y-x) / 2 is the best way to calculate mean.

@oscardssmith
Copy link
Member

llvm should be able to get rid of the floating point division since it's by a constant power of 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors to Julia kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants