add hypot fallback for 2 Numbers of the same type
#34316
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Seems like this is causing some subtyping crash when creating the sysimage? |
28 tasks
giordano
reviewed
Jan 9, 2020
base/math.jl
Outdated
| @@ -613,6 +613,7 @@ julia> hypot(3, 4im) | |||
| hypot(x::Number, y::Number) = hypot(promote(x, y)...) | |||
| hypot(x::Complex, y::Complex) = hypot(abs(x), abs(y)) | |||
| hypot(x::T, y::T) where {T<:Real} = hypot(float(x), float(y)) | |||
| hypot(x::T, y::T) where {T<:Number} = sqrt(abs2(x) + abs2(y)) | |||
There was a problem hiding this comment.
Doesn't this overflow/underflow, which hypot should not do?
There was a problem hiding this comment.
At least use something like abs(x) * sqrt (1 + (y/x)^2).
|
Probably not worth holding up the PR. |
|
Why not just revert that PR and keep the old method for non-AbstractFloats? |
|
There already is a method for two Reals and two Complexes. So this would only apply to non-Real, non-Complex Numbers, so potentially somewhat exotic types that might not support all operations. |
JeffBezanson
force-pushed
the
jb/hypotfallback
branch
from
59ce7c4
to
56fc981
Compare
KristofferC
pushed a commit
that referenced
this pull request
Jan 13, 2020
KristofferC
pushed a commit
that referenced
this pull request
Jan 15, 2020
KristofferC
pushed a commit
that referenced
this pull request
Jan 17, 2020
JeffBezanson
pushed a commit
that referenced
this pull request
Feb 21, 2020
KristofferC
pushed a commit
that referenced
this pull request
Feb 21, 2020
26 tasks
birm
pushed a commit
to birm/julia
that referenced
this pull request
Feb 22, 2020
Fix hypot with both arguments 0 Remove hypot from Furlongs in order to test the defined fallback
KristofferC
pushed a commit
that referenced
this pull request
Apr 11, 2020
KristofferC
pushed a commit
that referenced
this pull request
Apr 11, 2020
BioTurboNick
pushed a commit
to BioTurboNick/julia
that referenced
this pull request
Apr 13, 2020
(cherry picked from commit ade4713)
BioTurboNick
pushed a commit
to BioTurboNick/julia
that referenced
this pull request
Apr 13, 2020
Fix hypot with both arguments 0 Remove hypot from Furlongs in order to test the defined fallback (cherry picked from commit 8e3aa66)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#33224 broke
hypotwith Unitful.jl. There used to be a method for this. The weird thing is, in the current state it works if you pass 3 arguments tohypotbut not 2, so we should probably add this.