Skip to content

Make conversions work for Float16 (and speed tests)#43

Merged
timholy merged 4 commits intomasterfrom
teh/float16
Jul 21, 2016
Merged

Make conversions work for Float16 (and speed tests)#43
timholy merged 4 commits intomasterfrom
teh/float16

Conversation

@timholy
Copy link
Copy Markdown
Member

@timholy timholy commented Jul 21, 2016

Curiously, currently we have this:

julia> using FixedPointNumbers

julia> x = UFixed8(0.3)
UFixed8(0.298)

julia> y = convert(Float16, x)
0.29803923f0

julia> typeof(y)
Float32

As @vchuravy knows well, that may soon be fixed on julia master (see JuliaLang/julia#17297), but since this package also supports julia 0.4 it seems sensible to fix it here.

I also was reminded about how awfully slow the tests are. Poking at this a bit, I discovered that changing two characters (see 06b6372) speeds up the tests by several fold. Moreover, I suspect there's more improvement possible, because this loop isn't type-stable, even on julia-0.5 (the type of fun is changing on each iteration). However, I confess I'm having trouble understanding the purpose of that loop: as far as I can tell, it only ever runs the tests for fun == /. @vchuravy, can you shed some light on this? I'm guessing you meant to run the two lines with @test for all 4 functions, skipping it for / when fy == 0, right?

@timholy
Copy link
Copy Markdown
Member Author

timholy commented Jul 21, 2016

OK, I pushed two more commits and have gotten the tests from ~10min to ~20s. @vchuravy, please check to see whether you think these changes are correct.

@vchuravy
Copy link
Copy Markdown
Collaborator

Thanks, the for loop was indeed a mistake. @test is really slower than @assert?

This looks good. lgtm

@timholy
Copy link
Copy Markdown
Member Author

timholy commented Jul 21, 2016

Much of the performance improvement came from other changes, but the @test->@assert change also had a pretty big impact.

This branch:

julia> @time include("runtests.jl")
# <output suppressed>
 19.473147 seconds (93.47 M allocations: 1.910 GB, 1.71% gc time)

If I drop the last commit that changed @test to @assert:

julia> @time include("runtests.jl")
# <output suppressed>
144.961808 seconds (3.16 G allocations: 143.512 GB, 11.94% gc time)

I'd say that's enough of an improvement to be worth it. If you disagree, I'm happy to drop the last commit.

@vchuravy
Copy link
Copy Markdown
Collaborator

No that looks worth it. Thank you for this.

@timholy timholy merged commit b58c7f0 into master Jul 21, 2016
@timholy timholy deleted the teh/float16 branch July 21, 2016 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants