Move obvious type piracy from Hecke to Nemo #1501

lgoettgens · 2023-06-28T13:46:17Z

I will have a look through the Hecke files that contain many cases of reported type piracy, and move those functions to Nemo, that are either trivial in their implementation, or if it is clear that they only use functionality already in Nemo (like calls to flint).

Functions, where I do not directly see a good place to put them, will be in some files named after the Hecke file they originate from for the time being. Moving them around in Nemo is easy to do later in a separate PR. Or if you have any recommendations, let me know and I will move them in this PR.

The then duplicate functions will be removed from Hecke at thofma/Hecke.jl#1143.

fingolfin · 2023-06-28T15:05:40Z

src/HeckeMiscInteger.jl

+    return c < 0
+end
+
+function mulmod(a::UInt, b::UInt, n::UInt, ni::UInt)


There are probably better places for this and other functions. What's the plan here? Move the first into this file and merge it; and then when we are done, we can take our time to move things to a nicer place?

Functions, where I do not directly see a good place to put them, will be in some files named after the Hecke file they originate from for the time being. Moving them around in Nemo is easy to do later in a separate PR. Or if you have any recommendations, let me know and I will move them in this PR.

src/flint/fmpz.jl?

fingolfin · 2023-06-28T15:06:44Z

Maybe it would be best to organize a meeting (real or virtual) involving at least @lgoettgens @thofma and @fieker (I am also happy to join) to closely coordinate these changes and do them in 1-2 days, instead of having all these disruptive changes spaced out over multiple weeks and PRs and so on....

lgoettgens · 2023-06-28T15:35:45Z

Maybe it would be best to organize a meeting (real or virtual) involving at least @lgoettgens @thofma and @fieker (I am also happy to join) to closely coordinate these changes and do them in 1-2 days, instead of having all these disruptive changes spaced out over multiple weeks and PRs and so on....

That sounds like a good idea. I am available to travel to Kaiserslautern or Siegen for a few days. Or meet online. Let's further plan this on slack.

fingolfin

A couple more semi-random suggestions. I'd like to stress that these are just meant as food for thought. I don't mind merging the PR w/o them (of course there are other blockers for merging, like tests need to pass and syncing with our other packages be sorted out).

fingolfin · 2023-06-28T16:32:23Z

src/HeckeMiscInteger.jl

+function remove(z::T, p::T) where {T<:Integer}
+    z == 0 && return (0, z)
+    v = 0
+    @assert p > 1
+    while mod(z, p) == 0
+        z = Base.div(z, p)
+        v += 1
+    end
+    return (v, z)
+end
+
+function remove(z::Rational{T}, p::T) where {T<:Integer}
+    z == 0 && return (0, z)
+    v, d = remove(denominator(z), p)
+    w, n = remove(numerator(z), p)
+    return w - v, n // d
+end
+
+function valuation(z::T, p::T) where {T<:Integer}
+    z == 0 && return 0
+    v = 0
+    @assert p > 1
+    while mod(z, p) == 0
+        z = Base.div(z, p)
+        v += 1
+    end
+    return v
+end
+
+function valuation(z::Rational{T}, p::T) where {T<:Integer}
+    z == 0 && error("Not yet implemented")
+    v = valuation(denominator(z), p)
+    w = valuation(numerator(z), p)
+    return w - v
+end


These could even go into AbstractAlgebra, no?

I do not know if it is easier to manage large PRs over three repos right now or first move stuff from Hecke to Nemo and then later, in a separate PR, tackle the Nemo <-> AbstractAlgebra side. For the moment, I am doing the later.

~~Changed my plan. There now is Nemocas/AbstractAlgebra.jl#1392 containing the parts of this PR that belong to AA.~~

src/HeckeMiscInteger.jl

fingolfin · 2023-06-28T16:34:19Z

src/HeckeMiscInteger.jl

+function remove!(a::QQFieldElem, b::ZZRingElem)
+    nr = ccall((:fmpq_numerator_ptr, libflint), Ptr{ZZRingElem}, (Ref{QQFieldElem},), a)
+    vn = ccall((:fmpz_remove, libflint), Clong, (Ptr{ZZRingElem}, Ptr{ZZRingElem}, Ref{ZZRingElem}), nr, nr, b)
+    #QQFieldElem's are simplified: either num OR den will be non-trivial
+    if vn != 0
+        return vn, a
+    end
+    nr = ccall((:fmpq_denominator_ptr, libflint), Ptr{ZZRingElem}, (Ref{QQFieldElem},), a)
+    vn = ccall((:fmpz_remove, libflint), Clong, (Ptr{ZZRingElem}, Ptr{ZZRingElem}, Ref{ZZRingElem}), nr, nr, b)
+    return -vn, a
+end
+
+function valuation!(a::QQFieldElem, b::ZZRingElem)
+    nr = ccall((:fmpq_numerator_ptr, libflint), Ptr{ZZRingElem}, (Ref{QQFieldElem},), a)
+    vn = ccall((:fmpz_remove, libflint), Clong, (Ptr{ZZRingElem}, Ptr{ZZRingElem}, Ref{ZZRingElem}), nr, nr, b)
+    #QQFieldElem's are simplified: either num OR den will be non-trivial
+    if vn != 0
+        return vn
+    end
+    nr = ccall((:fmpq_denominator_ptr, libflint), Ptr{ZZRingElem}, (Ref{QQFieldElem},), a)
+    vn = ccall((:fmpz_remove, libflint), Clong, (Ptr{ZZRingElem}, Ptr{ZZRingElem}, Ref{ZZRingElem}), nr, nr, b)
+    return -vn
+end
+
+function BigFloat(a::QQFieldElem)
+    r = BigFloat(0)
+    ccall((:fmpq_get_mpfr, libflint), Nothing, (Ref{BigFloat}, Ref{QQFieldElem}, Int32), r, a, __get_rounding_mode())
+    return r
+end
+
+function isless(a::Float64, b::QQFieldElem)
+    return a < b * 1.0
+end
+function isless(a::QQFieldElem, b::Float64)
+    return a * 1.0 < b
+end


belongs to src/flint/fmpq.jl...

Though I guess we could also move all the methods involving BigFloat inputs into a file of their own... a matter of taste, I am open to either or a third alternative.

src/HeckeMiscInteger.jl

fingolfin · 2023-06-29T07:16:09Z

src/HeckeMiscInteger.jl

+    return c < 0
+end
+
+function mulmod(a::UInt, b::UInt, n::UInt, ni::UInt)


src/flint/fmpz.jl?

src/HeckeMiscInteger.jl

codecov · 2023-06-29T10:08:35Z

Codecov Report

Merging #1501 (2819441) into master (2942627) will decrease coverage by 6.02%.
The diff coverage is 10.50%.

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   88.78%   82.76%   -6.02%     
==========================================
  Files          89       95       +6     
  Lines       34346    37181    +2835     
==========================================
+ Hits        30494    30773     +279     
- Misses       3852     6408    +2556

Impacted Files	Coverage Δ
src/Nemo.jl	`29.41% <ø> (ø)`
src/arb/acb.jl	`87.42% <0.00%> (-0.44%)`	⬇️
src/arb/arb.jl	`87.72% <0.00%> (-1.00%)`	⬇️
src/flint/fmpq.jl	`85.60% <0.00%> (-9.18%)`	⬇️
src/HeckeMoreStuff.jl	`3.20% <3.20%> (ø)`
src/HeckeMiscPoly.jl	`7.05% <7.05%> (ø)`
src/HeckeMiscMatrix.jl	`13.59% <13.59%> (ø)`
src/flint/fmpz.jl	`91.03% <15.38%> (-2.74%)`	⬇️
src/HeckeMiscInteger.jl	`16.10% <16.10%> (ø)`
src/HeckeMiscFiniteField.jl	`38.59% <38.59%> (ø)`
... and 1 more

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lgoettgens · 2023-06-29T16:49:22Z

The number of reported piracy cases already went down from 721 to 425!

src/HeckeMiscMatrix.jl

lgoettgens · 2023-07-03T08:49:38Z

The OscarCI-release failures are expected as this is a breaking change.
The OscarCI-matching-1.8 failures will be fixed by oscar-system/Oscar.jl#2442.
~~As the OscarCI-matching-1.6 succeeded, I think this is good to go.~~

lgoettgens · 2023-07-03T16:25:19Z

~~The now relevant CI for everything working is the OscarCI-matching in Nemocas/AbstractAlgebra.jl#1392.~~

lgoettgens · 2023-07-03T20:56:06Z

The move of some stuff to AbstractAlgebra seemed too involved for me to fix it in the next days. Thus I would revert back to the original plan to just move a lot of stuff from Hecke to Nemo in this PR, and look to Nemo and AA sometime in the future.
I guess we will already have enough fun with the other breaking changes in AA for the next release.

Co-authored-by: Max Horn <[email protected]>

This reverts commit 67d2f74.

lgoettgens mentioned this pull request Jun 28, 2023

Move obvious type piracy from Hecke to Nemo thofma/Hecke.jl#1143

Merged

lgoettgens force-pushed the lg/hecke-piracy branch 2 times, most recently from 6175631 to bd539b3 Compare June 28, 2023 14:07

fingolfin reviewed Jun 28, 2023

View reviewed changes

fingolfin reviewed Jun 29, 2023

View reviewed changes

lgoettgens force-pushed the lg/hecke-piracy branch from bd539b3 to 0c66dcc Compare June 29, 2023 09:57

lgoettgens force-pushed the lg/hecke-piracy branch 3 times, most recently from 9020a7d to ebf791e Compare June 29, 2023 15:42

lgoettgens force-pushed the lg/hecke-piracy branch 3 times, most recently from 2a83a9e to 93fba0a Compare July 1, 2023 17:09

fingolfin reviewed Jul 2, 2023

View reviewed changes

src/HeckeMiscMatrix.jl Outdated Show resolved Hide resolved

lgoettgens marked this pull request as ready for review July 3, 2023 08:49

lgoettgens mentioned this pull request Jul 3, 2023

Move some obvoius type piracy from Hecke/Nemo to AA Nemocas/AbstractAlgebra.jl#1392

Closed

lgoettgens force-pushed the lg/hecke-piracy branch from 324e250 to 67d2f74 Compare July 3, 2023 16:35

lgoettgens marked this pull request as draft July 3, 2023 16:37

lgoettgens marked this pull request as ready for review July 4, 2023 15:12

lgoettgens mentioned this pull request Jul 6, 2023

Fix some fallout of fixing piracy in Hecke oscar-system/Oscar.jl#2523

Merged

lgoettgens added 4 commits July 6, 2023 11:30

Move obvious piracy from Hecke/src/misc/Integer.jl to Nemo

cad4e19

Address comments

f2e7b9f

Move obvious piracy from Hecke/src/misc/Matrix.jl to Nemo

02db004

Move obvious piracy from Hecke/src/misc/Poly.jl to Nemo

fbd7488

lgoettgens and others added 13 commits July 6, 2023 11:30

Move more obvious piracy from Hecke to Nemo

557dfe6

Move obvious piracy from Hecke/src/misc/FiniteField.jl to Nemo

f89f0fa

Move obvious piracy from Hecke/src/misc/Integer.jl to Nemo

5601c18

Move more obvious piracy from Hecke to Nemo

cb17250

Add some missing exports

eeb8b55

Move much more obvious piracy from Hecke to Nemo

e844def

Some fixes

16f36a4

More stuff moved from Hecke

6133c29

Adapt to thofma/Hecke.jl#1138

6b64a36

Update src/HeckeMiscMatrix.jl

a2e6a5e

Co-authored-by: Max Horn <[email protected]>

Remove some stuff now in AbstractAlgebra

afa987c

Revert "Remove some stuff now in AbstractAlgebra"

26d942f

This reverts commit 67d2f74.

Apply change from thofma/Hecke.jl#1152

a53fe90

lgoettgens force-pushed the lg/hecke-piracy branch from 8149883 to 2819441 Compare July 6, 2023 09:30

Adapt to thofma/Hecke.jl#1094

2819441

thofma merged commit 513e097 into Nemocas:master Jul 6, 2023

lgoettgens deleted the lg/hecke-piracy branch July 6, 2023 10:02

lgoettgens mentioned this pull request Jul 6, 2023

convenience functions for the oscar book thofma/Hecke.jl#1146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move obvious type piracy from Hecke to Nemo #1501

Move obvious type piracy from Hecke to Nemo #1501

lgoettgens commented Jun 28, 2023 •

edited

Loading

fingolfin Jun 28, 2023

lgoettgens Jun 28, 2023

fingolfin Jun 29, 2023

fingolfin commented Jun 28, 2023

lgoettgens commented Jun 28, 2023

fingolfin left a comment

fingolfin Jun 28, 2023

lgoettgens Jun 29, 2023

lgoettgens Jul 3, 2023 •

edited

Loading

fingolfin Jun 28, 2023

fingolfin Jun 29, 2023

codecov bot commented Jun 29, 2023 •

edited

Loading

lgoettgens commented Jun 29, 2023

lgoettgens commented Jul 3, 2023 •

edited

Loading

lgoettgens commented Jul 3, 2023 •

edited

Loading

lgoettgens commented Jul 3, 2023 •

edited

Loading

Move obvious type piracy from Hecke to Nemo #1501

Move obvious type piracy from Hecke to Nemo #1501

Conversation

lgoettgens commented Jun 28, 2023 • edited Loading

fingolfin Jun 28, 2023

Choose a reason for hiding this comment

lgoettgens Jun 28, 2023

Choose a reason for hiding this comment

fingolfin Jun 29, 2023

Choose a reason for hiding this comment

fingolfin commented Jun 28, 2023

lgoettgens commented Jun 28, 2023

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jun 28, 2023

Choose a reason for hiding this comment

lgoettgens Jun 29, 2023

Choose a reason for hiding this comment

lgoettgens Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

fingolfin Jun 28, 2023

Choose a reason for hiding this comment

fingolfin Jun 29, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

lgoettgens commented Jun 29, 2023

lgoettgens commented Jul 3, 2023 • edited Loading

lgoettgens commented Jul 3, 2023 • edited Loading

lgoettgens commented Jul 3, 2023 • edited Loading

lgoettgens commented Jun 28, 2023 •

edited

Loading

lgoettgens Jul 3, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

lgoettgens commented Jul 3, 2023 •

edited

Loading

lgoettgens commented Jul 3, 2023 •

edited

Loading

lgoettgens commented Jul 3, 2023 •

edited

Loading