Skip to content
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

Fix div/divrem #868

Merged
merged 5 commits into from
Aug 14, 2020
Merged

Fix div/divrem #868

merged 5 commits into from
Aug 14, 2020

Conversation

wbhart
Copy link
Contributor

@wbhart wbhart commented Aug 12, 2020

This makes the internal definition of div/divrem in Nemo agree with the internal definition used by AbstractAlgebra (and with Flint).

This allows generic code to run correctly over Nemo.fmpz (e.g. the Generic.Modules test that I added as a regression test).

Fixes #842
Fixes #743 (because it was annoying me)

N.B. This is breaking and should not be merged until we have checked that no other packages break. In any event, at least Hecke.jl and Oscar.jl (and maybe Singular.jl) should import Nemo.div and Nemo.divrem instead of Base.div and Base.divrem, which will likely cause breakage.

I will check this out as soon as possible.

@thofma
Copy link
Member

thofma commented Aug 12, 2020

Does this mean Base.div(::fmpz, ::fmpz) and Nemo.div(::fmpz, ::fmpz) are different? So in Nemo and in the REPL the behaviors are different? This makes working with the only useful integer type that we have more cumbersome, just to cope with some questionable design desicions for BigInt, which no one should use anyway.

@thofma
Copy link
Member

thofma commented Aug 12, 2020

Let's continue this at #842.

@wbhart
Copy link
Contributor Author

wbhart commented Aug 12, 2020

It's already different in AbstractAlgebra.

I have opened many tickets discussing possible solutions to this problem. There aren't really any options left.

src/Nemo.jl Outdated Show resolved Hide resolved
@thofma
Copy link
Member

thofma commented Aug 12, 2020

I will make a new patch release with the new flint version (once it is merged upstream) and then we can merge this here.

@thofma thofma merged commit 39f5716 into Nemocas:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modules over Nemo.ZZ fail due to divrem/div definitions *Massive* regression in nf_elem adhoc binary tests
2 participants