More conformance tests, fix some corner cases

[fingolfin]

No description provided.

[fingolfin]

This reports what looks like a genuine issue but I did not yet have time to look into it. Specifically fpRelPowerSeriesRingElem.conformance_tests fails with

  Expression: base_ring_type(R) == typeof(base_ring(R))
   Evaluated: zzModRing == fpField

[lgoettgens]

I cherry-picked #1884 on top of this branch to see what failure CI runs into next

[lgoettgens]

The next failure is due to a wrong neg! method in #1869. This branch would need to get rebased for us to even be able to fix it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.84%. Comparing base (16f4530) to head (6f7d06a).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1883      +/-   ##
==========================================
+ Coverage   87.49%   87.84%   +0.35%     
==========================================
  Files          97       97              
  Lines       35537    35534       -3     
==========================================
+ Hits        31092    31215     +123     
+ Misses       4445     4319     -126     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lgoettgens]

The CI failures still unveil some issue with gcdinv for fpPolyRing

[fingolfin]

Weird, tests pass for me locally. But it seems in CI they also only fail in some runs, so probably a rare random input triggers this? sigh

[lgoettgens]

Yeah, something with some of the arguments being 0.

…

On October 18, 2024 12:39:45 PM GMT+02:00, Max Horn ***@***.***> wrote: Weird, tests pass for me locally. But it seems in CI they also only fail in some runs, so probably a rare random input triggers this? *sigh* -- Reply to this email directly or view it on GitHub: #1883 (comment) You are receiving this because you commented. Message ID: ***@***.***>

[lgoettgens]

I boiled the problem down to the following:
It happens if the first argument of gcdinv is a multiple of the second. In this case, the definitions of AA and FLINT disagree:
AA states that the first return value is always the gcd, while FLINT states that if the first argument is zero modulo the second it will return 0 as both return values.
So this will need another (sigh) check in https://github.com/nemocas/Nemo.jl/blob/c2d3375041be823ee38a6f5ebbb21df37c7d466e/src/flint/gfp_poly.jl#L236 and in the analogous one for FpPolyRingElem...

[fingolfin]

I guess in that case there are even more issues, just not discovered by the tests. E.g:

julia> gcdinv(25, 5)
(5, 0)

julia> gcdinv(ZZ(25), ZZ(5))
ERROR: DomainError with (25, 5):
First argument must be smaller than second argument
Stacktrace:
 [1] gcdinv(a::ZZRingElem, b::ZZRingElem)
   @ Nemo ~/Projekte/OSCAR/Nemo.jl/src/flint/fmpz.jl:1410
 [2] top-level scope
   @ REPL[18]:1

The documentation for gcdinv in FLINT is insufficient, I've submitted flintlib/flint#2104 now.

In the meantime I think the definition we have in AA at least is simple. But we could of course augment it to say something like "if f is zero mod g then the return value is undefined"

[fingolfin]

Should be ready now. Found several true bugs in the code where we produced incorrect results or errors

[fingolfin]

Oh and I just removed two ccalls into FLINT gcdinv methods. Claus and me inspected the FLINT implementation and they basically avoid some allocations. But at the same time their API is completely underspecified, and trying to make them work sanely is not worth the hassle right now. See also flintlib/flint#2104