Add exhaustive divrem tests for ZZ

[lgoettgens]

Before I touch the implementations, I wanted to make sure to capture the current behavior.

Assumption: Base.divrem(x,y) == (Base.div(x,y), Base.rem(x,y) and Nemo.divrem(x,y) == (Nemo.div(x,y), Base.mod(x,y))

The few commented out tests are due to some weirdness with finite precision Ints together with unsiged Ints. All of the offending methods are in AA, and even in julia base are some discussions about what the return type of these should be.

One method I had to remove (there is a fallback via UInt -> ZZRingElem), because it did not satisfy the above assumption.

Does this look sensible?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.21%. Comparing base (633112d) to head (26fd198).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1919      +/-   ##
==========================================
- Coverage   87.24%   87.21%   -0.03%     
==========================================
  Files          97       97              
  Lines       35590    35588       -2     
==========================================
- Hits        31049    31037      -12     
- Misses       4541     4551      +10     

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

[joschmitt]

I think questions about such fundamental conventions are above my pay grade, sorry!

[fieker]

On Wed, Oct 23, 2024 at 09:06:59AM -0700, Lars Göttgens wrote: Before I touch the implementations, I wanted to make sure to capture the current behavior. Assumption: `Base.divrem(x,y) == (Base.div(x,y), Base.rem(x,y)` and `Nemo.divrem(x,y) == (Nemo.div(x,y), Base.mod(x,y))`

Good assumption - cannot be guarateend in general, unfortunately. For rings with zero divisors (Z/nZ, ZK/Ideal, ...) the division is non-unique and, in particular for ZK/Ideal, non-deterministic: Hecke/src/NumFieldOrd/NfOrd/ResidueRing.jl so, even if div and mod are implemented using divrem, since divrem is random, the test will fail. % is mapped to rem I think? Thus the removal of the troubling code will kill ZZ(3) % UInt(2) This is total insanity - I wasn't aware of Nemo defining both Nemo.divrem and Base.divrem Outside of Nemo.jl I do not think anyone uses Nemo.divrem Question for me would then rather be: can we remove it? If we want the other pair, isn't there a "proper" Julia approach with a "RoundingMode" to signal rem/ mod?

…

The few commented out tests are due to some weirdness with finite precision Ints together with unsiged Ints. All of the offending methods are in AA, and even in julia base are some discussions about what the return type of these should be. One method I had to remove (there is a fallback via `UInt -> ZZRingElem`), because it did not satisfy the above assumption. Does this look sensible? You can view, comment on, or merge this pull request online at: #1919 -- Commit Summary -- * Add exhaustive `divrem` tests for ZZ * Remove troubling method -- File Changes -- M src/flint/fmpz.jl (4) M test/flint/fmpz-test.jl (87) -- Patch Links -- https://github.com/Nemocas/Nemo.jl/pull/1919.patch https://github.com/Nemocas/Nemo.jl/pull/1919.diff -- Reply to this email directly or view it on GitHub: #1919 You are receiving this because your review was requested. Message ID: ***@***.***>

[fieker]

... Maybe rename Nemo.divrem into divmod?

[fieker]

as this will remove ZZ(3) % UInt(2) I don't like it. Rather do an error if the input is illegal (ie. the remainder negative)