-
Notifications
You must be signed in to change notification settings - Fork 59
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
Unify rounding code #1540
Unify rounding code #1540
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1540 +/- ##
==========================================
+ Coverage 84.03% 84.19% +0.16%
==========================================
Files 94 94
Lines 36614 36614
==========================================
+ Hits 30767 30826 +59
+ Misses 5847 5788 -59
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
f936119
to
8a192a9
Compare
This PR got a bit out of hand 😅 but it should be there now. |
It seems that code coverage for the changes is below 50%? Is this a fluke? |
90f256b
to
b37bd2d
Compare
Part of this PR is moving a lot of untested code around. While I added some tests for new stuff I added, I did not e.g. add tests for the various But I've now taken the time to add some (and of course once again found and fixed issues in the existing untested code 🙄 ). |
@@ -215,35 +215,8 @@ function (::ZZRing)(x::Rational{Int}) | |||
return ZZRingElem(numerator(x)) | |||
end | |||
|
|||
function ceil(::Type{ZZRingElem}, a::BigFloat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW this is now handled by generic code (in the macro) which also supports e.g. Float64 and more
Systematically provide methods for ceil, floor, round, trunc with various source and/or target types we provide. Remove some (accidental?) type piracy. For `m` a `Matrix{BigFloat}` instead of `trunc(m)` one now needs to write `trunc(ZZMatrix, m)` which avoids type piracy. On the upside, this is now supported for any `Matrix{<:Real}`. Also add methods `is_positive(::QQFieldElem)` and `is_negative(::QQFieldElem)`, move some functions to more appropriate places, and "optimize" (?) `sign(::Type{Int}, a::QQFieldElem)`.
The Oscar error looks real: https://github.com/Nemocas/Nemo.jl/actions/runs/6411567510/job/17444653341?pr=1540 Any idea what is going on? |
The return value of the Old:
vs new:
|
To mimic the behavior of Base for Float, I would say that the new behavior is better (cf https://docs.julialang.org/en/v1/base/math/#Base.round-Tuple{Type,%20Any}). |
The documentation of
That But of course I can reverse that -- or mark this PR as "breaking" and we just update callsites, I don't think there'll be many, and the fix is trivial (just add |
Let's mark this breaking and fix the callsites when bumping the dependency. |
OK. I am going to release AA now, then Nemo, then... we'll see ;-) |
I am seeing a ton of new errors reported by Aqua in my CI when it runs with the new version of Nemo. If I understand correctly, it due to the changes made here. |
Systematically provide methods for ceil, floor, round, trunc with various source and/or target types we provide. Remove some (accidental?) type piracy.
For
m
aMatrix{BigFloat}
instead oftrunc(m)
one now needs to writetrunc(ZZMatrix, m)
which avoids type piracy. On the upside, this is now supported for anyMatrix{<:Real}
.Also add methods
is_positive(::QQFieldElem)
andis_negative(::QQFieldElem)
, move some functions to more appropriate places, and "optimize" (?)sign(::Type{Int}, a::QQFieldElem)
.See also #1495