Move pirated stuff from Nemo #1412

lgoettgens · 2023-09-04T10:53:46Z

Counterpart to Nemocas/Nemo.jl#1531. This will most certainly be a breaking change.

codecov · 2023-09-04T11:15:38Z

Codecov Report

Merging #1412 (20f1e14) into master (c28e89c) will decrease coverage by 3.85%.
Report is 9 commits behind head on master.
The diff coverage is 0.52%.

❗ Current head 20f1e14 differs from pull request most recent head 783603c. Consider uploading reports for the commit 783603c to get more accurate results

@@            Coverage Diff             @@
##           master    #1412      +/-   ##
==========================================
- Coverage   85.80%   81.95%   -3.85%     
==========================================
  Files         104      107       +3     
  Lines       28190    26632    -1558     
==========================================
- Hits        24188    21827    -2361     
- Misses       4002     4805     +803

Files Changed	Coverage Δ
src/AbstractAlgebra.jl	`39.79% <ø> (-16.46%)`	⬇️
src/NemoStuff.jl	`0.52% <0.52%> (ø)`

... and 80 files with indirect coverage changes

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

fingolfin · 2023-09-04T12:13:24Z

src/NemoStuff.jl

@@ -1,5 +1,7 @@
 export neg!

+sub!(z::T, x::T, y::T) where {T} = x - y


Should there be a similar method for add! and/or mul!?

Note that in src/NCRings.jl we already have this:

function zero!(z::NCRingElem) return zero(parent(z)) end function add!(z::T, x::T, y::T) where T <: NCRingElem return x + y end function addeq!(z::T, y::T) where T <: NCRingElem return z + y end function sub!(z::T, x::T, y::T) where T <: NCRingElem return x - y end function mul!(z::T, x::T, y::T) where T <: NCRingElem return x*y end function addmul!(z::T, x::T, y::T, c::T) where T <: NCRingElem c = mul!(c, x, y) z = addeq!(z, c) return z end

So it seems this already does the same thing, the new code here "only" drops the requirement sub! (why? probably to support Int, Rational, etc.?)

I noticed that as well, but I would like to leave this PR simply as a copy-paste of stuff. There are a few similar things as well that should be cleaned up in a follow-up PR.

Sure.

But just to record my thoughts for the future: I suggest we drop this method here, and move those from src/NCRings.jl to src/fundamental_interface.jl, but making them generic, i.e. letting T be free.... Or indeed, letting all three arguments be "free": add!(x,a,b) = a+b etc.

fingolfin · 2023-09-04T13:10:48Z

Any idea why the diagonal_matrix doctest suddenly is failing?

src/NemoStuff.jl

lgoettgens · 2023-09-04T13:58:57Z

Any idea why the diagonal_matrix doctest suddenly is failing?

Flint's QQElems print differently than AbstractAlgebra's QQElems. And since flint is not available here, QQ refers to a different implementation of the rationals.

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

lgoettgens · 2023-09-04T14:21:57Z

GitHub seems to have some issues with tar files in CI. I will wait some time and then rerun all jobs.

fingolfin · 2023-09-05T10:10:58Z

Any idea why the diagonal_matrix doctest suddenly is failing?

Flint's QQElems print differently than AbstractAlgebra's QQElems. And since flint is not available here, QQ refers to a different implementation of the rationals.

Yeah, but the tests were already here and not failing, which confused me. But I guess your PR #1413 answers that... ;-)

fingolfin · 2023-09-05T10:13:16Z

I am wondering now, will this technically be a breaking change? We "only" add methods; sure, some are overwritten in Nemo (and perhaps elsewhere), but by themselves they should not break anything, right?

lgoettgens · 2023-09-05T10:42:53Z

I am wondering now, will this technically be a breaking change? We "only" add methods; sure, some are overwritten in Nemo (and perhaps elsewhere), but by themselves they should not break anything, right?

I broke at least vcat(Vector{MatElem}) for everyone using AbstractAlgebra and not Nemo. So yes, I would consider this breaking.

fingolfin

What's the plan, when will this be non-draft?

I propose that we merge & release soon, together with PR #1410, as 0.32, and then quickly proceed with updating Nemo, Hecke, Oscar, ...

(Unless e.g. @thofma or @fieker have objections?)

fingolfin · 2023-09-05T15:13:17Z

src/NemoStuff.jl

+is_negative(x::Rational) = x.num < 0
+
+is_negative(n::Integer) = cmp(n, 0) < 0
+is_positive(n::Integer) = cmp(n, 0) > 0


For a follow-up PR:

Suggested change

is_negative(x::Rational) = x.num < 0

is_negative(n::Integer) = cmp(n, 0) < 0

is_positive(n::Integer) = cmp(n, 0) > 0

is_negative(n::T) where T<:Real = n < zero(T)

is_positive(n::T) where T<:Real = n > zero(T)

Of course that ignores some icky questions, like: should is_negative(-0.0) be true (matching signbit(-0.0) == true) or false (matching -0.0 == 0.0 resp. iszero(-0.0) both holding)? But I think for our purposes the above makes most sense.

fingolfin · 2023-09-05T15:21:15Z

src/NemoStuff.jl

+function is_symmetric(M::MatElem)
+    for i in 1:nrows(M)
+        for j in i:ncols(M)
+            if M[i, j] != M[j, i]
+                return false
+            end
+        end
+    end
+    return true
+end


We already have the following in src/Matrix.jl:1302 -- these should be merged (probably by just dropping the restriction on T on the existing method)

function is_symmetric(a::MatrixElem{T}) where T <: NCRingElement if !is_square(a) return false end for row in 2:nrows(a) for col in 1:(row - 1) if a[row, col] != a[col, row] return false end end end return true end

fingolfin · 2023-09-05T15:22:30Z

src/NemoStuff.jl

+################################################################################
+
+@doc raw"""
+vcat(A::Vector{Mat}) -> Mat


Suggested change

vcat(A::Vector{Mat}) -> Mat

vcat(A::Vector{Mat}) -> Mat

fingolfin · 2023-09-05T15:33:51Z

src/NemoStuff.jl

+    return shift_left(a, -v)
+end
+
+# should be Nemo/AA


Suggested change

# should be Nemo/AA

fingolfin · 2023-09-05T15:34:47Z

src/NemoStuff.jl

+import LinearAlgebra
+LinearAlgebra.dot(a::RingElem, b::RingElem) = a * b
+
+function is_upper_triangular(M::MatElem)


To be merged with is_upper_triangular method in src/Matrix.jl:3359

fingolfin · 2023-09-05T15:35:15Z

src/NemoStuff.jl

+end
+
+function Base.rem(f::PolyRingElem, g::PolyRingElem)
+    return mod(f, g)


Huh, I wonder why not

Suggested change

return mod(f, g)

q, r = divrem(f, g)

return r

lgoettgens · 2023-09-05T16:14:52Z

What's the plan, when will this be non-draft?

Once all failures are resolved. I am going to ignore most of your comments for now. They can be addressed in a future PR (after the breaking release), first I want everything to work without further changes.

fingolfin · 2023-09-05T16:45:25Z

Yes, ignore the comments. I consider them as notes for a future PR

* `leading_monomial(::Generic.MPoly)` * `leading_coefficient(::Generic.MPoly)`

lgoettgens · 2023-09-05T17:13:32Z

src/NemoStuff.jl

-function leading_coefficient(f::Generic.MPoly)
-    return f.coeffs[1]
-end
-


This fails for the zero polynomial. AA already contains a better implementation.

lgoettgens · 2023-09-05T17:13:52Z

src/NemoStuff.jl

-function leading_monomial(f::Generic.MPoly)
-    R = parent(f)
-    l = length(f)
-    if l == 0
-        return f
-    end
-    A = f.exps
-    r, c = size(A)
-    e = A[1:r, 1:1]
-    return R([one(base_ring(R))], e)
-end
-


same here, fails for the zero poly

lgoettgens · 2023-09-05T17:15:05Z

test/generic/FunctionField-test.jl

-      test_rand(S, 1:10, -10:10)
+      @test_broken false # test_rand(S, 1:10, -10:10) # TODO: make this work again after #1412
   end
-   
+
   for f in P2
      S, y = FunctionField(f, "y")

      # TODO: test more than just the result type
-      test_rand(S, 1:10)
+      @test_broken false # test_rand(S, 1:10) # TODO: make this work again after #1412


I have no idea how and why this broke. If you have an idea, please let me know.

Argh I wrote a bunch of comments here and my browser died sigh. Let's see if I can cobble it back together:

As far as I can tell, there is already a problem here in master... but somehow there I can only trigger it when I handle the test file "interactively". That is, if I enter the following, I get an error also on master (I am assuming this code is executed from inside the AbstractAlgebra.jl directory:

using AbstractAlgebra, Test include("test/rand.jl") include("test/generic/FunctionField-test.jl")

this gives me:

... Test Summary: | Pass Total Time Generic.FunctionField.printing | 9 9 0.0s Generic.FunctionField.rand: Error During Test at /Users/mhorn/Projekte/OSCAR/AbstractAlgebra.jl/test/generic/FunctionField-test.jl:96 Got exception outside of a @test return type AbstractAlgebra.Generic.FunctionFieldElem{Rational{BigInt}} does not match inferred return type Any Stacktrace: [1] error(s::String) @ Base ./error.jl:35 [2] test_rand(::Union{Nothing, Function}, ::Any, ::Any, ::Vararg{Any}; type::Any) @ Main ~/Projekte/OSCAR/AbstractAlgebra.jl/test/rand.jl:32 [3] test_rand @ ~/Projekte/OSCAR/AbstractAlgebra.jl/test/rand.jl:18 [inlined] [4] #test_rand#9 @ ~/Projekte/OSCAR/AbstractAlgebra.jl/test/rand.jl:15 [inlined] [5] test_rand(::Any, ::Any, ::Any) @ Main ~/Projekte/OSCAR/AbstractAlgebra.jl/test/rand.jl:15 [6] macro expansion @ ~/Projekte/OSCAR/AbstractAlgebra.jl/test/generic/FunctionField-test.jl:101 [inlined] [7] macro expansion @ ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined] [8] top-level scope @ ~/Projekte/OSCAR/AbstractAlgebra.jl/test/generic/FunctionField-test.jl:97 [9] include(fname::String) @ Base.MainInclude ./client.jl:478 [10] top-level scope @ REPL[17]:1 [11] eval @ ./boot.jl:370 [inlined] [12] eval_user_input(ast::Any, backend::REPL.REPLBackend, mod::Module) @ REPL ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/REPL/src/REPL.jl:153 [13] repl_backend_loop(backend::REPL.REPLBackend, get_module::Function) @ REPL ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/REPL/src/REPL.jl:249 [14] start_repl_backend(backend::REPL.REPLBackend, consumer::Any; get_module::Function) @ REPL ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/REPL/src/REPL.jl:234 [15] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool, backend::Any) @ REPL ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/REPL/src/REPL.jl:379 [16] run_repl(repl::REPL.AbstractREPL, consumer::Any) @ REPL ~/.julia/juliaup/julia-1.9.3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.9/REPL/src/REPL.jl:365 [17] (::Base.var"#1017#1019"{Bool, Bool, Bool})(REPL::Module) @ Base ./client.jl:421 [18] #invokelatest#2 @ ./essentials.jl:819 [inlined] [19] invokelatest @ ./essentials.jl:816 [inlined] [20] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool) @ Base ./client.jl:405 [21] exec_options(opts::Base.JLOptions) @ Base ./client.jl:322 [22] _start() @ Base ./client.jl:522 Test Summary: | Error Total Time Generic.FunctionField.rand | 1 1 0.1s ERROR: LoadError: Some tests did not pass: 0 passed, 0 failed, 1 errored, 0 broken. in expression starting at /Users/mhorn/Projekte/OSCAR/AbstractAlgebra.jl/test/generic/FunctionField-test.jl:96

And this I tracked down further to

using AbstractAlgebra, Test using AbstractAlgebra.RandomExtensions: RandomExtensions, make R = polynomial_ring(polynomial_ring(QQ,:x1)[1], :y)[1] @inferred make(R, 0:0, 1:10, -10:10)

But @code_warntype make(R, 0:0, 1:10, -10:10) reveals nothing, no code ambiguity? It also infers the correct result type. Huh, weird.

PR #1420 fixes this, so in theory after merging this we can re-run the tests here and hopefully they'll pass

Now that #1420 is merged, I reverted the commenting out. Let's hope it succeeds now.

lgoettgens · 2023-09-05T17:17:22Z

AbstractAlgebra tests now succeed locally on julia-1.9. Let's see if that's the case here as well, and the OscarCI-matching jobs as well.

thofma · 2023-09-05T17:31:03Z

I am happy with a quick 0.32 release.

This reverts commit 4063ccb.

fingolfin

Fine by me, thanks!

The NegInf stuff could be moved in a follow-up PR, if you are willing. Then we can merge #1410 and #1419 and then quickly make a breaking release, then update Nemo etc....

lgoettgens · 2023-09-11T14:16:59Z

Fine with me, let's get this going before there arise conflicts. Infinity stuff goes in a new PR

lgoettgens · 2023-09-12T12:44:46Z

@fingolfin Auto-merge failed as the OscarCI-matching job here has a different name than the required one.

Move stuff, part 1

8909eb1

lgoettgens mentioned this pull request Sep 4, 2023

Move pirated stuff to AbstractAlgebra Nemocas/Nemo.jl#1531

Merged

lgoettgens added 3 commits September 4, 2023 13:18

Move stuff, part 2

3317911

Move stuff, part 3

9ca2ddf

Move stuff, part 4

2aa6c50

fingolfin reviewed Sep 4, 2023

View reviewed changes

lgoettgens added 2 commits September 4, 2023 14:27

Fix doctest

d65dd47

Remove contradicting implementations of remove and valuation

d1e2a43

fingolfin reviewed Sep 4, 2023

View reviewed changes

src/NemoStuff.jl Outdated Show resolved Hide resolved

src/NemoStuff.jl Show resolved Hide resolved

src/NemoStuff.jl Outdated Show resolved Hide resolved

lgoettgens and others added 2 commits September 4, 2023 16:07

Fix some tests

b620528

Apply suggestions from code review

1bb6c66

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

lgoettgens closed this Sep 4, 2023

lgoettgens reopened this Sep 4, 2023

lgoettgens closed this Sep 4, 2023

lgoettgens reopened this Sep 4, 2023

lgoettgens closed this Sep 4, 2023

lgoettgens reopened this Sep 4, 2023

Remove broken matrix cat tests

2be883f

fingolfin added breaking and removed breaking labels Sep 5, 2023

fingolfin added the breaking label Sep 5, 2023

fingolfin reviewed Sep 5, 2023

View reviewed changes

lgoettgens added 2 commits September 5, 2023 19:08

Comment out broken rand test

4063ccb

Remove contradicting implementations

663bfe8

* `leading_monomial(::Generic.MPoly)` * `leading_coefficient(::Generic.MPoly)`

lgoettgens commented Sep 5, 2023

View reviewed changes

lgoettgens marked this pull request as ready for review September 5, 2023 17:15

lgoettgens closed this Sep 11, 2023

lgoettgens reopened this Sep 11, 2023

Revert "Comment out broken rand test"

20f1e14

This reverts commit 4063ccb.

fingolfin approved these changes Sep 11, 2023

View reviewed changes

fingolfin enabled auto-merge (squash) September 11, 2023 14:23

auto-merge was automatically disabled September 12, 2023 09:42
Head branch was pushed to by a user without write access

Breaking version bump

783603c

fingolfin merged commit 723898c into Nemocas:master Sep 12, 2023
12 of 13 checks passed

lgoettgens deleted the lg/nemo-piracy branch September 12, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pirated stuff from Nemo #1412

Move pirated stuff from Nemo #1412

lgoettgens commented Sep 4, 2023

codecov bot commented Sep 4, 2023 •

edited

Loading

fingolfin Sep 4, 2023

lgoettgens Sep 4, 2023

fingolfin Sep 4, 2023

fingolfin commented Sep 4, 2023

lgoettgens commented Sep 4, 2023

lgoettgens commented Sep 4, 2023

fingolfin commented Sep 5, 2023

fingolfin commented Sep 5, 2023

lgoettgens commented Sep 5, 2023

fingolfin left a comment

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

fingolfin Sep 5, 2023

lgoettgens commented Sep 5, 2023

fingolfin commented Sep 5, 2023

lgoettgens Sep 5, 2023

lgoettgens Sep 5, 2023

lgoettgens Sep 5, 2023

fingolfin Sep 7, 2023

fingolfin Sep 11, 2023

lgoettgens Sep 11, 2023

lgoettgens commented Sep 5, 2023

thofma commented Sep 5, 2023

fingolfin left a comment

lgoettgens commented Sep 11, 2023

lgoettgens commented Sep 12, 2023

		@@ -1,5 +1,7 @@
		export neg!

		sub!(z::T, x::T, y::T) where {T} = x - y

Move pirated stuff from Nemo #1412

Move pirated stuff from Nemo #1412

Conversation

lgoettgens commented Sep 4, 2023

codecov bot commented Sep 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Sep 4, 2023

lgoettgens commented Sep 4, 2023

lgoettgens commented Sep 4, 2023

fingolfin commented Sep 5, 2023

fingolfin commented Sep 5, 2023

lgoettgens commented Sep 5, 2023

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Sep 5, 2023

fingolfin commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Sep 5, 2023

thofma commented Sep 5, 2023

fingolfin left a comment

Choose a reason for hiding this comment

lgoettgens commented Sep 11, 2023

lgoettgens commented Sep 12, 2023

codecov bot commented Sep 4, 2023 •

edited

Loading