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

Math tests: if fma is not available, relax some tests from exact equality to approximate equality #48102

Merged
merged 7 commits into from
Jan 6, 2023

Conversation

DilumAluthge
Copy link
Member

Ref #48101

@DilumAluthge DilumAluthge added test This change adds or pertains to unit tests maths Mathematical functions ci Continuous integration labels Jan 3, 2023
test/math.jl Outdated Show resolved Hide resolved
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@N5N3
Copy link
Member

N5N3 commented Jan 3, 2023

IIRC has_fma has no runtime support.
So we need a func wrapper to make this work

julia> f() = Core.Compiler.have_fma(Float64)
f (generic function with 1 method)

julia> f()
true

julia> Core.Compiler.have_fma(Float64)
false

test/math.jl Outdated Show resolved Hide resolved
@DilumAluthge DilumAluthge marked this pull request as ready for review January 3, 2023 18:53
@DilumAluthge DilumAluthge added the backport 1.9 Change should be backported to release-1.9 label Jan 3, 2023
# If the machine supports fma (fused multiply add), we require exact equality.
# Otherwise, we only require approximate equality.
if has_fma[T]
my_eq = (==)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use a different infix comparison operator to make this look nicer.

@StefanKarpinski
Copy link
Sponsor Member

Would it be slicker to have and overloaded infix operator here that chooses == or based on its argument types instead of all the if/else on type arguments?

@IanButterworth
Copy link
Sponsor Member

IanButterworth commented Jan 5, 2023

This appears to fix one Windows, and I just triggered a re-run for the other given it appeared to be a httpbingo blip.

If that succeeds and the suggested change above isn't imminent, it might be worth merging this as-is to get CI green again?

@IanButterworth
Copy link
Sponsor Member

httpbingo fail is still there, plus a few others. Also, windows tests reports are so noisy..

@DilumAluthge
Copy link
Member Author

At least it fixes x86_64-w64-mingw32.

Let's merge this so at least that platform will be green on master. And we can implement the above-discussed improvements in a follow-up PR.

@DilumAluthge DilumAluthge merged commit 6d14b0f into master Jan 6, 2023
@DilumAluthge DilumAluthge deleted the dpa/windows-fma-isapprox branch January 6, 2023 01:57
KristofferC pushed a commit that referenced this pull request Jan 10, 2023
…lity to approximate equality (#48102)

* Math tests: if fma is not available, relax some tests from exact equality to approximate equality

* Apply suggestions from code review

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* `has_fma` has no runtime support

* Add `Rational{Int}`

* Put the FMA support info in the testset context

* Fix whitespace

* Remove inaccurate testset name

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
(cherry picked from commit 6d14b0f)
@KristofferC KristofferC mentioned this pull request Jan 10, 2023
41 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration maths Mathematical functions test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants