Handle 32 bit architecture when creating float ranges (#22613) #22644

angusmoore · 2017-07-01T00:11:17Z

Adds a specialisation of maxintfloat that optionally checks (only on 32 bit systems) whether the max float is larger than maximum 32 bit integer. If so, it downsizes the returned float to the max Int32.

For background: The maximum 32 bit integer is smaller than the maximum representable integer in a Float64. This caused InexactErrors when linspace, floatrange, etc (see #22613) attempted to round the maximum float returned by maxintfloat to an Int32

I've added a test that reproduces the bug in #22613. This test passes under this branch.

I have also removed a line that skipped some range tests on 32 bit architecture. I assume it was this bug was causing the tests to fail on 32-bit, but it just got commented out and added to the todo pile. With the fix in this PR, these tests passed when I ran them on my 32-bit machine.

…n Int32.

ararslan · 2017-07-01T00:14:24Z

base/twiceprecision.jl

@@ -107,6 +107,9 @@ end
 function floatrange(a::AbstractFloat, st::AbstractFloat, len::Real, divisor::AbstractFloat)
 T = promote_type(typeof(a), typeof(st), typeof(divisor))
 m = maxintfloat(T)


Presumably it's this function that should rather be adjusted?

Hm, maybe not. At any rate, you could use a helper function to avoid having to recheck this every time you call maxintfloat, e.g.

_maxintfloat(T::Type) = (m = maxintfloat(T); Int === Int32 ? min(m, Float64(typemax(Int))) : m)

(or some better name)

Yes, that's a more code-efficient way of doing the check. I'll update this PR along those lines.

Sounds great. Thanks for working on this!

…erflow on 32 bit.

ararslan · 2017-07-01T06:09:37Z

base/floatfuncs.jl

@@ -20,6 +20,7 @@ maxintfloat(::Type{Float64}) = 9007199254740992.
 maxintfloat(::Type{Float32}) = Float32(16777216.)
 maxintfloat(::Type{Float16}) = Float16(2048f0)
 maxintfloat(x::T) where {T<:AbstractFloat} = maxintfloat(T)
+maxintfloat(::Type{S}, ::Type{T}) where S <: AbstractFloat where T <: Integer = (m = maxintfloat(S); Int === Int32 ? min(m, S(typemax(T))) : m)


The typical style for single line function definitions with type parameters is to use a single where with curly braces, e.g.

maxintfloat(::Type{S}, ::Type{T}) where {S<:AbstractFloat, T<:Integer} = ...

Ah, thank you. I wasn't sure of the syntax for this, and couldn't find many examples elsewhere.

Yeah, it's not really obvious in the documentation and it's hard to know where to look for inspiration in the code. Hopefully we'll find a way to improve the docs in that regard. At any rate, what you have here does work, the shorter form is just preferred for one-liners.

It's good to know, thanks. Looking at it, I prefer the curly brace way because it's much easier to read; I'll default to it in future. I've changed the PR to that style.

ararslan · 2017-07-01T06:09:48Z

base/twiceprecision.jl

- if Int == Int32
- m = min(m, 2147483647.)
- end
+ m = maxintfloat(T ,Int)


Got the comma and space permuted here

ararslan

Looks good to me. Great work here! I've also requested a review from Tim Holy, who knows float ranges far better than I. 🙂

angusmoore · 2017-07-01T06:32:02Z

Thanks very much @ararslan, really appreciate your friendly help on my first PR!

timholy

With one small exception, LGTM. Thanks for contributing this!

timholy · 2017-07-01T08:46:52Z

base/floatfuncs.jl

@@ -20,6 +20,7 @@ maxintfloat(::Type{Float64}) = 9007199254740992.
 maxintfloat(::Type{Float32}) = Float32(16777216.)
 maxintfloat(::Type{Float16}) = Float16(2048f0)
 maxintfloat(x::T) where {T<:AbstractFloat} = maxintfloat(T)
+maxintfloat(::Type{S}, ::Type{T}) where {S<:AbstractFloat, T<:Integer} = (m = maxintfloat(S); Int === Int32 ? min(m, S(typemax(T))) : m)


So, if someone types maxintfloat(Float64, Int16) then I think they'll say this is producing the wrong answer.

The good news is you don't have to work so hard, because LLVM takes care of optimizing this for you:

julia> mymaxintfloat(::Type{S}, ::Type{T}) where {S<:AbstractFloat, T<:Integer} = min(maxintfloat(S), S(typemax(T))) mymaxintfloat (generic function with 1 method) julia> mymaxintfloat(Float64, Int32) 2.147483647e9 julia> mymaxintfloat(Float64, Int) 9.007199254740992e15 julia> @code_llvm mymaxintfloat(Float64, Int32) define double @julia_mymaxintfloat_60880(i8**, i8**) #0 !dbg !5 { top: ret double 0x41DFFFFFFFC00000 } julia> mymaxintfloat(Float64, Int16) 32767.0 julia> @code_llvm mymaxintfloat(Float64, Int16) define double @julia_mymaxintfloat_60922(i8**, i8**) #0 !dbg !5 { top: ret double 3.276700e+04 }

Thanks Tim. I had considered adding the extra generality, but wasn't sure whether it would slow down the code. Turns out I should have trusted LLVM's cleverness even more. Thanks for these suggestions, very informative!

… generic.

tkelman · 2017-07-02T00:01:51Z

base/floatfuncs.jl

@@ -20,6 +20,7 @@ maxintfloat(::Type{Float64}) = 9007199254740992.
 maxintfloat(::Type{Float32}) = Float32(16777216.)
 maxintfloat(::Type{Float16}) = Float16(2048f0)
 maxintfloat(x::T) where {T<:AbstractFloat} = maxintfloat(T)
+maxintfloat(::Type{S}, ::Type{T}) where {S<:AbstractFloat, T<:Integer} = min(maxintfloat(S), S(typemax(T)))


may be worth documenting and testing this new signature (including the corner cases that Tim pointed out), since it's exported if it's using the same name

Thanks, good point @tkelman. I've added tests, and I think added documentation (I'm not especially familiar with Documenter, but it passed make docs and I think looks right).

tkelman · 2017-07-02T13:42:37Z

base/docs/helpdb/Base.jl

+"""
+ maxintfloat(T, S)
+
+The largest integer losslessly representable by the given floating-point DataType `T` that also does not exceed the maximum integer representable by the integer data type T.


we prefer to wrap long lines in inline docstrings

the integer data type is the second input right? so one should say S with code highlighting

tkelman · 2017-07-02T13:43:45Z

test/floatfuncs.jl

@@ -19,6 +19,10 @@ for elty in (Float16,Float32,Float64)
 @test maxintfloat(rand(elty)) == maxintfloat(elty)
 end
 @test maxintfloat() == maxintfloat(Float64)
+@test maxintfloat(Float64, Int32) == 2147483647.0


these could use === which would also test that the return type is as expected

nalimilan · 2017-07-03T07:57:10Z

test/ranges.jl

 l = linspace(start,stop,n)
 @test n == length(l)
 # FIXME: these fail some small portion of the time
 @test_skip start == first(l)
 @test_skip stop == last(l)
 end

+# Inexact errors on 32 bit architectures. #22613
+@test first(linspace(log(0.2), log(10.0), 10)) == log(0.2)
+@test last(linspace(log(0.2), log(10.0), 10)) == log(10.0)


Probably also worth testing floatrange(-3e9, 1.0, 1, 1.0).

Fair point. I hadn't originally because I incorrectly had in my mind that floatrange had been deprecated (I was thinking of FloatRange). I've added a simple test for the bug you identified in the original issue.

ararslan · 2017-07-12T01:40:19Z

Good to merge?

angusmoore · 2017-07-12T01:54:42Z

Yes, the previous appveyor test failure looked like a timeout, if this one passes, I think this is ready to be merged.

timholy · 2017-07-12T17:26:34Z

Thanks a ton, @angusmoore! Very nice work.

This seems likely to fix JuliaPlots/Plots.jl#968. Since it sounds like it's not possible to run Plots on 32-bit machines, this might be a high-priority backport.

tkelman · 2017-07-13T00:21:44Z

probably should have squash-merged this

timholy · 2017-07-13T02:11:13Z

Argh, yes indeed, my apologies.

Handle 32 bit architecture when creating float ranges (#22613) Ref #22613 (cherry picked from commit f6e55a7)

Ref #22644 (cherry picked from commit b9034e7) (cherry picked from commit dd4d8eb) (cherry picked from commit a07f767) (cherry picked from commit f502e1c) (cherry picked from commit ca60ff5) (cherry picked from commit 5018fd6) (cherry picked from commit 8713fa3) (cherry picked from commit 7772517) (cherry picked from commit 4de1a51) (cherry picked from commit 7f163ba)

angusmoore added 3 commits June 30, 2017 16:08

Added simple tests for #22613.

b9034e7

Reenable range tests on 32 bit architecture.

dd4d8eb

Address int rounding error in maxintfloat by explicitly checking if o…

a07f767

…n Int32.

ararslan reviewed Jul 1, 2017

View reviewed changes

ararslan added collections Data structures holding multiple items, e.g. sets system:32-bit Affects only 32-bit systems backport pending 0.6 labels Jul 1, 2017

Create a specialisation of maxintfloat that checks the float won't ov…

f502e1c

…erflow on 32 bit.

ararslan reviewed Jul 1, 2017

View reviewed changes

Style and whitespace corrections.

ca60ff5

ararslan requested a review from timholy July 1, 2017 06:24

ararslan approved these changes Jul 1, 2017

View reviewed changes

timholy requested changes Jul 1, 2017

View reviewed changes

Made the version of maxintfloat that checks against typemax(int) more…

5018fd6

… generic.

tkelman reviewed Jul 2, 2017

View reviewed changes

timholy approved these changes Jul 2, 2017

View reviewed changes

Tests and documentation for maxintfloat(Float, Integer).

8713fa3

tkelman reviewed Jul 2, 2017

View reviewed changes

angusmoore added 2 commits July 3, 2017 10:38

Fix line wrapping, formatting and typo in maxintfloat doc.

7772517

Convert maxintfloat tests to use ===

4de1a51

nalimilan reviewed Jul 3, 2017

View reviewed changes

Add extra test from #22613 for floatrange.

7f163ba

angusmoore changed the title ~~RFC: Handle 32 bit architecture when creating float ranges (#22613)~~ Handle 32 bit architecture when creating float ranges (#22613) Jul 12, 2017

timholy mentioned this pull request Jul 12, 2017

InexactError() on windows with the 32bit version, Julia 0.7-dev JuliaPlots/Plots.jl#968

Closed

timholy merged commit f6e55a7 into JuliaLang:master Jul 12, 2017

angusmoore deleted the anm/ranges-32bit-#22613 branch July 13, 2017 00:11

angusmoore mentioned this pull request Jul 13, 2017

floatrange() overflows for some inputs on 32-bit #22613

Closed

daschw mentioned this pull request Jul 13, 2017

InexactError() on Linux 32bit, Julia 0.6 JuliaPlots/Plots.jl#963

Closed

andreasnoack mentioned this pull request Jul 24, 2017

InexactError on Julia 0.6 32-bit Windows JuliaStats/StatsBase.jl#288

Closed

abelsiqueira mentioned this pull request Aug 1, 2017

Create dixmaanj as a SimpleNLPModel. Drop AmplNLReader JuliaSmoothOptimizers/SolverTools.jl#42

Merged

ararslan pushed a commit that referenced this pull request Sep 11, 2017

Merge pull request #22644 from angusmoore/anm/ranges-32bit-#22613

115d105

Handle 32 bit architecture when creating float ranges (#22613) Ref #22613 (cherry picked from commit f6e55a7)

ranocha mentioned this pull request Sep 30, 2017

added recipe for SavedValues and fixed bug for ManifoldProjection if autodiff==false SciML/DiffEqCallbacks.jl#21

Merged

ararslan removed the backport pending 0.6 label Oct 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle 32 bit architecture when creating float ranges (#22613) #22644

Handle 32 bit architecture when creating float ranges (#22613) #22644

angusmoore commented Jul 1, 2017 •

edited

Loading

ararslan Jul 1, 2017

ararslan Jul 1, 2017 •

edited

Loading

angusmoore Jul 1, 2017

ararslan Jul 1, 2017

ararslan Jul 1, 2017

angusmoore Jul 1, 2017

ararslan Jul 1, 2017

angusmoore Jul 1, 2017

ararslan Jul 1, 2017

ararslan left a comment

angusmoore commented Jul 1, 2017

timholy left a comment

timholy Jul 1, 2017

angusmoore Jul 1, 2017

tkelman Jul 2, 2017

angusmoore Jul 2, 2017

tkelman Jul 2, 2017

tkelman Jul 2, 2017

nalimilan Jul 3, 2017

angusmoore Jul 3, 2017

ararslan commented Jul 12, 2017

angusmoore commented Jul 12, 2017

timholy commented Jul 12, 2017

tkelman commented Jul 13, 2017

timholy commented Jul 13, 2017

Handle 32 bit architecture when creating float ranges (#22613) #22644

Handle 32 bit architecture when creating float ranges (#22613) #22644

Conversation

angusmoore commented Jul 1, 2017 • edited Loading

Choose a reason for hiding this comment

ararslan Jul 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan left a comment

Choose a reason for hiding this comment

angusmoore commented Jul 1, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Jul 12, 2017

angusmoore commented Jul 12, 2017

timholy commented Jul 12, 2017

tkelman commented Jul 13, 2017

timholy commented Jul 13, 2017

angusmoore commented Jul 1, 2017 •

edited

Loading

ararslan Jul 1, 2017 •

edited

Loading