restore v0.4 behavior on v0.5, fixes #880 #884
Conversation
| @@ -327,7 +327,7 @@ function apply_statistic(stat::HistogramStatistic, | |||
| x_min = Gadfly.concrete_minimum(values) | |||
| span = x_max - x_min | |||
| binwidth = span / d | |||
| bincounts ./= sum(bincounts) * binwidth | |||
| bincounts = bincounts ./ sum(bincounts) * binwidth | |||
There was a problem hiding this comment.
I thought this was safe. See #864
There's one more occurrence of this same line in this file. Does that work OK?
There was a problem hiding this comment.
It's not. This line causes an InexactError in the case that binwidth is a float. bincounts is always an integer array and with the new meaning of ./= on v0.5 we are trying to do an in-place update of integer array using floats causing a type coercion error, i.e. the InexactError.
julia v0.4:
julia> a = [1,2,3]
3-element Array{Int64,1}:
1
2
3
julia> a ./= 2.0
3-element Array{Float64,1}:
0.5
1.0
1.5julia v0.5-rc3
julia> a = [1,2,3]
3-element Array{Int64,1}:
1
2
3
julia> a ./= 2.0
ERROR: InexactError()
in copy!(::Base.LinearFast, ::Array{Int64,1}, ::Base.LinearFast, ::Array{Float64,1}) at ./abstractarray.jl:559
in broadcast!(::Base.#identity, ::Array{Int64,1}, ::Array{Float64,1}) at ./broadcast.jl:24There was a problem hiding this comment.
good detective work!
@stevengj pinging you to let you know about this.
There was a problem hiding this comment.
I think that is the intended behavior.
There was a problem hiding this comment.
(In-place, fused vectorized operations will be a key feature in Julia 0.6. The fact that it catches a type instability is a bonus. 😉)
There was a problem hiding this comment.
Note that your new 0.4-style code is still type-unstable. If I were you, I'd use a different variable name (binmean?) for the normalized bincounts.
There was a problem hiding this comment.
Note that your new 0.4-style code is still type-unstable.
All of Gadfly is type-unstable 😂 haha.
If I were you, I'd use a different variable name (
binmean?) for the normalizedbincounts.
Why is having a new variable better? binmean will still sometimes be an Int sometimes a Float64.
There was a problem hiding this comment.
No, I mean do binmean = bincounts ./ sum(bincounts) .* binwidth. Then bincounts is always an array of integers, and binmean is always an array of floats. In subsequent lines, work with binmean.
(I realize that it is probably not performance critical to be type-stable here, but it is better style in Julia to retain type stability if it is not too hard.)
also add test for histogram with density=true
|
I wasn't able to get the other |
f3ba121 to
b9e8975
Compare
|
wow that build hasn't started in a good 4 hours. |
|
Yeah, at least tests are passing on nightlies now that JuliaStats/KernelDensity.jl#36 was resolved. Would it be possible to merge this? Tests pass on Linux on Travis and they pass on my local OS X machine. It would be nice for #879 |
|
Thanks @tkelman! That explains why |
also add test for histogram with density=true