JuliaIntervals / IntervalLinearAlgebra.jl

Linear algebra done rigorously
MIT License
36 stars 9 forks source link

CONSISTENT_FPCSR #132

Closed orkolorko closed 1 year ago

orkolorko commented 1 year ago

PR description

On x86_64 we switch to OpenBLAS with CONSISTENT_FPCSR flag enabled to guarantee coherent floating point rounding over all BLAS thread. For other architectures we fall back to only one thread.

Related issues

Checklist

[X] Updated Project.toml

Other

I'm worried about the fact that the artifact is only available for x86_64 architectures, but is a dependency of the package. I have no experience of this fact, so I do not know if I need to find a solution for this problem or not.

codecov-commenter commented 1 year ago

Codecov Report

Base: 97.19% // Head: 96.56% // Decreases project coverage by -0.63% :warning:

Coverage data is based on head (2ea3de1) compared to base (fe87dd4). Patch coverage: 82.14% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #132 +/- ## ========================================== - Coverage 97.19% 96.56% -0.63% ========================================== Files 17 18 +1 Lines 642 670 +28 ========================================== + Hits 624 647 +23 - Misses 18 23 +5 ``` | [Impacted Files](https://codecov.io/gh/JuliaIntervals/IntervalLinearAlgebra.jl/pull/132?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIntervals) | Coverage Δ | | |---|---|---| | [src/IntervalLinearAlgebra.jl](https://codecov.io/gh/JuliaIntervals/IntervalLinearAlgebra.jl/pull/132?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIntervals#diff-c3JjL0ludGVydmFsTGluZWFyQWxnZWJyYS5qbA==) | `73.68% <66.66%> (-26.32%)` | :arrow_down: | | [src/numerical\_test/multithread.jl](https://codecov.io/gh/JuliaIntervals/IntervalLinearAlgebra.jl/pull/132?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIntervals#diff-c3JjL251bWVyaWNhbF90ZXN0L211bHRpdGhyZWFkLmps) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIntervals). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIntervals)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lucaferranti commented 1 year ago

Thanks! I'll have a closer look later today, but overall looks good to me.

I'm worried about the fact that the artifact is only available for x86_64 architectures, but is a dependency of the package

This means that e.g. on mac M1 it wouldnt work? Ideally we should make this architecture dependent dependency, not sure if it's possible (I'll try to have a look).

orkolorko commented 1 year ago

It seems that the CI failing is independent from the PR. What should I do?

One more thing: maybe it is worth writing tests that test that effectively CONSISTENT_FPCSR does what it says; setting this flag is what is suggested by INTLAB documentation but it would be good to have some in house tests that test if everything works as it should.

orkolorko commented 1 year ago

Thanks! I'll have a closer look later today, but overall looks good to me.

I'm worried about the fact that the artifact is only available for x86_64 architectures, but is a dependency of the package

This means that e.g. on mac M1 it wouldnt work? Ideally we should make this architecture dependent dependency, not sure if it's possible (I'll try to have a look).

It should work also for aarch64, but it failed at compilation on Yggdrasil. The compiler did not recognize the assembler codes used to switch rounding mode in the OpenBLAS source code; since I have no access to an aarch64 it is kind of difficult for me to understand the issue and I do not feel comfortable in trying to correct this in the OpenBLAS source.

lucaferranti commented 1 year ago

I'll think for a couple of days if I know someone who could test on aarch, otherwise we can just merge with a warning and wait to see if/when someone encounters issues