Nemo.jl icon indicating copy to clipboard operation
Nemo.jl copied to clipboard

Improve comparision for ZZRingElem and QQFieldElem; add some missing `fits` methods and make others faster; and more

Open fingolfin opened this issue 1 month ago • 2 comments

All changes were carefully benchmarked, but there is too much and I am too tired to include details. But many things got faster by a factor 2 of this -- and in practice one can hope for even more, because ccalls were replaced by direct code; the Julia compiler can inline and optimize the latter but not the former.

fingolfin avatar Nov 06 '25 21:11 fingolfin

Codecov Report

:x: Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.18%. Comparing base (f9ccb66) to head (6aa8343).

Files with missing lines Patch % Lines
src/flint/fmpq.jl 66.66% 5 Missing :warning:
src/flint/fmpz.jl 96.42% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2170      +/-   ##
==========================================
- Coverage   88.19%   88.18%   -0.01%     
==========================================
  Files          99       99              
  Lines       37190    37191       +1     
==========================================
- Hits        32798    32796       -2     
- Misses       4392     4395       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Nov 06 '25 21:11 codecov[bot]

Test error is

Test Failed at /home/runner/work/Nemo.jl/Nemo.jl/oscar-dev/Oscar/test/Rings/integer.jl:357
  Expression: Int(ZZ(typemin(Int)) - 1)
    Expected: InexactError
  No exception thrown

which indeed should throw I think.

thofma avatar Nov 07 '25 08:11 thofma

The error was caused by mixing up <= and >= sigh.

And I copied all those ZZ tests from Oscar to here. I really don't understand why they are in Oscar in the first place?!

fingolfin avatar Nov 25 '25 23:11 fingolfin

To make sure that ZZ and QQ construct things of the right type, but it appears we then added some more.

thofma avatar Nov 26 '25 06:11 thofma