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

hypot for Float16 regressed #38311

Closed
achuchmala opened this issue Nov 5, 2020 · 6 comments · Fixed by #38322
Closed

hypot for Float16 regressed #38311

achuchmala opened this issue Nov 5, 2020 · 6 comments · Fixed by #38322
Assignees
Labels
regression Regression in behavior compared to a previous version
Milestone

Comments

@achuchmala
Copy link
Contributor

achuchmala commented Nov 5, 2020

I changed the commit as suggested in PR #38272, however some issue arised during running tests with make command:

ach@ach-Vostro-7580:~/development/julia/packages/julia$ make test-math
    JULIA test/math
Test  (Worker) | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
math       (1) |        started at 2020-11-05T01:59:33.535
math       (1) |         failed at 2020-11-05T02:00:11.506
Test Failed at /home/ach/development/julia/packages/julia/test/math.jl:1173
  Expression: #= /home/ach/development/julia/packages/julia/test/math.jl:1173 =# @inferred(hypot(x, complex(x / 4))) ≈ x * sqrt(17 / BigFloat(16))
   Evaluated: Inf16 ≈ 33759.98886255740458193770390071574268190526725735920411685601169650148618781252

the issue is with Float16, when I remove Float16 from the tested types, all tests pass.

When I do the same in REPL, everything goes as it should:

julia> using Test

julia> x = floatmax(Float16)/2
Float16(3.275e4)

julia> @test (@inferred hypot(x, complex(x/4))) ≈ x * sqrt(17/BigFloat(16))
Test Passed

Interestingly, the test in line below passes even with Float16:

@test (@inferred hypot(x, complex(x/4), x/4)) ≈ x * sqrt(9/BigFloat(8))

all test pass.

It seems that there is something wrong with make command.

Version Info

julia> versioninfo()
Julia Version 1.5.2
Commit 539f3ce943 (2020-09-23 23:17 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)
Environment:
  JULIA_PKG_SERVER = pkg.juliahub.com
  JULIA_DEPOT_PATH = /home/ach/.julia:/opt/JuliaPro-1.5.2-1/Julia/local/share/julia:/opt/JuliaPro-1.5.2-1/Julia/share/julia
  JULIA_LOAD_PATH = @:@v#.#:@stdlib
@vchuravy
Copy link
Member

vchuravy commented Nov 5, 2020

The version info shown is from 1.5.2 Julia 1.6 is changing the handling of Float16 (cc: @maleadt ) Do you see the same discrepancy between the REPL and the testsuite if both are in 1.6?

@achuchmala
Copy link
Contributor Author

When I run tests in REPL in 1.6 I get have the same situation as in testsuite:

julia> using Test

julia> x = floatmax(Float16)/2
Float16(3.275e4)

julia> @test (@inferred hypot(x, complex(x/4))) ≈ x * sqrt(17/BigFloat(16))
Test Failed at REPL[3]:1
  Expression: #= REPL[3]:1 =# @inferred(hypot(x, complex(x / 4))) ≈ x * sqrt(17 / BigFloat(16))
   Evaluated: Inf16 ≈ 33759.98886255740458193770390071574268190526725735920411685601169650148618781252
ERROR: There was an error during testing

julia> versioninfo()
Julia Version 1.6.0-DEV.1400
Commit b89bfd90a7* (2020-11-02 11:54 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.0 (ORCJIT, skylake)

As I good understand, when I invoke make command in the folder with current git repo it uses the 1.6 version? If it is the case, then it seems that something is broken with Float16 in v1.6.

@vchuravy
Copy link
Member

vchuravy commented Nov 5, 2020

Ok that makes more sense :)

@JeffBezanson JeffBezanson added the regression Regression in behavior compared to a previous version label Nov 5, 2020
@JeffBezanson JeffBezanson added this to the 1.6 features milestone Nov 5, 2020
@vchuravy vchuravy changed the title test for hypot with Float16 arguments doesn't pass if run by make command hypot for Float16 regressed Nov 5, 2020
@vchuravy
Copy link
Member

vchuravy commented Nov 5, 2020

So the change between 1.5 and 1.6 is that 1.5 defined:

        hypot(a::Float16,b::Float16) = Float16(hypot(Float32(a),Float32(b)))

This was removed with #38158 (so #37510 is not to blame), looking at the implementation of hypot the issue is that the scale value goes to Inf16 cc: @cfborges

@simonbyrne
Copy link
Contributor

The issue appears to be this:

julia> scale = eps(typeof(ax))*sqrt(floatmin(ax))  #Rescaling constant
Float16(7.6e-6)

julia> scale = inv(scale)
Inf16

@cfborges
Copy link
Contributor

cfborges commented Nov 5, 2020

Yup. It's happening because of the uneven exponent range in the format that gives you extended range for small numbers by reducing the range for large numbers. The code was developed to be format independent but still had to make assumptions that are fairly typical in FPS.

simonbyrne added a commit that referenced this issue Nov 5, 2020
simonbyrne added a commit that referenced this issue Dec 1, 2020
stevengj pushed a commit that referenced this issue Dec 4, 2020
* Fix overflow for hypot in Float16

Fixes #38311

* hypot tests for ax > floatmax(ax)/2

* hypot Float16 promote to Float32

Co-authored-by: achuchmala <ach@spyro-soft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants