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

Add Float16 comparisons #29916

Merged
merged 5 commits into from
Dec 13, 2018
Merged

Add Float16 comparisons #29916

merged 5 commits into from
Dec 13, 2018

Conversation

sam0410
Copy link
Contributor

@sam0410 sam0410 commented Nov 3, 2018

All the discussions corresponding to this PR is in #29870
Fixes issue #29743

base/float.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

stevengj commented Nov 8, 2018

Seems like you could use a loop for this kind of repetitive definition. For example:

for op in (:==, :<, :>)
    @eval begin
        $op(x::Union{Float16,Float32}, y::Union{Int128,UInt128}) = $op(Float64(x), Float64(y))
        $op(x::Union{Int128,UInt128}, y::Union{Float16,Float32}) = $op(Float64(x), Float64(y))
    end
end

@sam0410
Copy link
Contributor Author

sam0410 commented Nov 8, 2018

Hi @stevengj, I removed the @eval because of the following comment by @andyferris :
#29870 (comment)

Or would it be better if I add the for loop for a more readable code?

@stevengj
Copy link
Member

stevengj commented Nov 8, 2018

If @eval can replace many lines of repetitive code with a much shorter loop, it is a good idea. The comment by @andyferris was referring to cases where there is little or no code savings.

base/float.jl Outdated Show resolved Hide resolved
@sam0410
Copy link
Contributor Author

sam0410 commented Nov 9, 2018

The build is failing because of some 32bit issue, not because of the code in this PR.

test/numbers.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

stevengj commented Nov 10, 2018

The AppVeyor failure is an inference failure on 32-bit Windows. It's not completely clear to me that it is unrelated — are we seeing the same failure on other PRs?

@fredrikekre
Copy link
Member

The AppVeyor failure is an inference failure on 32-bit Windows.

See #29923, happens on most PRs

@sam0410
Copy link
Contributor Author

sam0410 commented Dec 6, 2018

Hi, Is this PR ready to be merged ? Or is there something else I could change?

@StefanKarpinski
Copy link
Member

Be sure to squash when merging.

@kshyatt
Copy link
Contributor

kshyatt commented Dec 12, 2018

CI ran out of logspace. This still ok to squash and merge?

@StefanKarpinski
Copy link
Member

Rerunning CI to be sure.

@StefanKarpinski
Copy link
Member

@stevengj already approved, so if CI passes, someone please merge.

@fredrikekre fredrikekre added the needs news A NEWS entry is required for this change label Dec 12, 2018
@stevengj stevengj removed the needs news A NEWS entry is required for this change label Dec 13, 2018
@stevengj
Copy link
Member

@fredrikekre, I don't think a bugfix like this requires a NEWS item…?

@stevengj stevengj merged commit 1d3c371 into JuliaLang:master Dec 13, 2018
@sam0410 sam0410 deleted the F16-comparison branch December 13, 2018 14:32
@fredrikekre
Copy link
Member

Ok, thought it was MethodErrors before.

@fredrikekre fredrikekre added the bugfix This change fixes an existing bug label Dec 13, 2018
KristofferC pushed a commit that referenced this pull request Dec 13, 2018
* Add Float16 comparisons

* Add @eval

* Add union

* Add != to tests

(cherry picked from commit 1d3c371)
@KristofferC KristofferC mentioned this pull request Dec 13, 2018
52 tasks
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.0 and removed triage This should be discussed on a triage call labels Jan 31, 2019
@JeffBezanson JeffBezanson removed triage This should be discussed on a triage call triage backport pending 1.0 labels Jan 31, 2019
JeffBezanson pushed a commit that referenced this pull request Jun 6, 2019
* Add Float16 comparisons

* Add @eval

* Add union

* Add != to tests

(cherry picked from commit 1d3c371)
@KristofferC KristofferC mentioned this pull request Aug 26, 2019
55 tasks
KristofferC pushed a commit that referenced this pull request Aug 26, 2019
* Add Float16 comparisons

* Add @eval

* Add union

* Add != to tests

(cherry picked from commit 1d3c371)
KristofferC pushed a commit that referenced this pull request Aug 27, 2019
* Add Float16 comparisons

* Add @eval

* Add union

* Add != to tests

(cherry picked from commit 1d3c371)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
* Add Float16 comparisons

* Add @eval

* Add union

* Add != to tests

(cherry picked from commit 1d3c371)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants