Added new func is_unimodular for square integer matrices #1991

JohnAAbbott · 2025-01-14T15:08:12Z

Added new func is_unimodular for square integer matrices

codecov · 2025-01-14T15:15:49Z

Codecov Report

Attention: Patch coverage is 96.59091% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.14%. Comparing base (6e20099) to head (f6c2d67).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
src/matrix.jl	96.57%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1991      +/-   ##
==========================================
+ Coverage   88.08%   88.14%   +0.05%     
==========================================
  Files          98       98              
  Lines       36031    36207     +176     
==========================================
+ Hits        31737    31913     +176     
  Misses       4294     4294

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

JohnAAbbott · 2025-01-15T13:37:45Z

I am not planning to make further changes to increase test coverage (currently 96%).

thofma · 2025-01-16T11:04:17Z

There are some issues, for example, the use of floating point numbers here:

if det_mod_p > p/2

and plenty of unnecessary Nemo. and ;.

JohnAAbbott · 2025-01-16T12:04:45Z

OK, mostly minor syntactic quibbles, right? The code should be mathematically sound.
I didn't know that floats are verboten. Semicolons are needed to separate commands on a single line, and when commands go strongly hand-in-hand it makes sense to put them together on one line (IMHO).

thofma · 2025-01-16T12:14:09Z

The 2^56 looks a bit wrong. Should probably be something like 2^(7 * sizeof(Int))?

JohnAAbbott · 2025-01-16T12:25:43Z

What is our stance on supporting platforms which are not 64-bit? I'd guess that 32-bit is probably "impossible" and/or pointless; or do we really support Int == Int32? Maybe we support 128-bit?
Perhaps I'd prefer something like (sizeof(Int) == 8) ? 2^56 : error("unsupported platform").
On my computer here computations with 21-22 bit primes were quick, but there was already a significant penalty for 23-bit primes: it was then better to jump to much larger primes (57-bit worked well on my computer). Ideally there would be an automatic "tuning" phase where these values are computed at start-up in Oscar....

fieker · 2025-01-16T12:30:28Z

On Thu, Jan 16, 2025 at 03:04:39AM -0800, Tommy Hofmann wrote: There are some issues, for example, the use of floating point numbers here: ``` if det_mod_p > p/2 ```

Why is this a problem? This is to get a symmetric lift to char 0...

and plenty of unnecessary `Nemo.` and `;`.

This is in and merged - not because it is perfect, but to get on with it. It will have to be refactored further as it is part of ongoing research (e.g. Michael J. recent addition)...

…

-- Reply to this email directly or view it on GitHub: #1991 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

thofma · 2025-01-16T12:50:23Z

  for i=1:nrows(A)
    A_p = Nemo.mat_entry_ptr(A, i, i)
    sub!(A_p, A_p, m)
  end

this needs a GC.@preserve and similar for the other function.

Why is this a problem? This is to get a symmetric lift to char 0...

I was thinking of things like this

julia> (2^61 + 1) > (2^62 + 2)/2
true

julia> (2^61 + 1) > (2^62 + 2)//2
false

Or I might have mistaken the intended use of floating point stuff here.

fieker · 2025-01-16T12:57:33Z

On Thu, Jan 16, 2025 at 04:50:46AM -0800, Tommy Hofmann wrote: ``` for i=1:nrows(A) A_p = Nemo.mat_entry_ptr(A, i, i) sub!(A_p, A_p, m) end ``` this needs a ***@***.***` and similar for the other function. > Why is this a problem? This is to get a symmetric lift to char 0... I was thinking of things like this ``` julia> (2^61 + 1) > (2^62 + 2)/2 true julia> (2^61 + 1) > (2^62 + 2)//2 false ``` Or I might have mistaken the intended use of floating point stuff here.

This is, hopefully, outside the use. The primes should be, depending n the type, <23 bits or <60 bits, but point taken. (Also: I have vague feeling it is irrelevant for the correctness - as we increase our understanding)

…

-- Reply to this email directly or view it on GitHub: #1991 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

fieker · 2025-01-16T12:58:16Z

On Thu, Jan 16, 2025 at 04:50:46AM -0800, Tommy Hofmann wrote: ``` for i=1:nrows(A) A_p = Nemo.mat_entry_ptr(A, i, i) sub!(A_p, A_p, m) end ``` this needs a ***@***.***` and similar for the other function.

We'll deal with this in the next iteration - there are a bevy of similar functions still under processing

…

> Why is this a problem? This is to get a symmetric lift to char 0... I was thinking of things like this ``` julia> (2^61 + 1) > (2^62 + 2)/2 true julia> (2^61 + 1) > (2^62 + 2)//2 false ``` Or I might have mistaken the intended use of floating point stuff here. -- Reply to this email directly or view it on GitHub: #1991 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

fingolfin

Didn't manage to comment on this in time, sorry. It would be good if some of these were still addressed, though

fingolfin · 2025-01-16T13:24:57Z

src/matrix.jl

+function Nemo.mul!(A::ZZMatrix, m::ZZRingElem)
+  for i=1:nrows(A)
+    A_p = Nemo.mat_entry_ptr(A, i, 1)
+    for j=1:ncols(A)
+      mul!(A_p, A_p, m)
+      A_p += sizeof(Int)
+    end
+  end
+  return A
+end


But we already define such a method in src/flint/fmpz_mat.jl:

function mul!(z::ZZMatrixOrPtr, a::ZZMatrixOrPtr, b::ZZRingElemOrPtr) @ccall libflint.fmpz_mat_scalar_mul_fmpz(z::Ref{ZZMatrix}, a::Ref{ZZMatrix}, b::Ref{ZZRingElem})::Nothing return z end

(and then generic code in AA also makes mul!(::ZZMatrix, ::ZZRingElem) available.

fingolfin · 2025-01-16T13:25:01Z

src/matrix.jl

+# add_verbosity_scope(:UnimodVerif) -- see func __init__ in src/Nemo.jl
+
+#######################################################
+# Low-level auxiliary functions -- should be elsewhere?


Yeah, I would say most of the coded added here should actually be in src/flint/fmpz_mat.jl

fingolfin · 2025-01-16T13:27:02Z

src/matrix.jl

+  return r
+end
+
+function map_entries!(a::fpMatrix, k::Nemo.fpField, A::ZZMatrix)


The argument order clashes with the general map_entries! signature as defined in AA which is map_entries!(f, dst::MatrixElem{T}, src::MatrixElem{U})

fingolfin · 2025-01-16T13:28:20Z

src/matrix.jl

+  if !is_square(A)
+    throw(ArgumentError("Matrix must be square"))
+  end


While this is fine, I'd recommend to use the @req macro liberally for such tests:

Suggested change

if !is_square(A)

throw(ArgumentError("Matrix must be square"))

end

@req is_square(A) "Matrix must be square"

fingolfin · 2025-01-16T13:30:00Z

src/matrix.jl

+    ZZmodP = Nemo.Native.GF(p; cached = false, check = false)
+    map_entries!(A_mod_p, ZZmodP, A)
+    @vtime :UnimodVerif 2   det_mod_p = Int(_det(A_mod_p))
+    if det_mod_p > p/2


While the cost for converting ints to float etc. here, it could indeed still be avoided here, e.g. via use of >>

Suggested change

if det_mod_p > p/2

if det_mod_p > p>>1

fingolfin · 2025-01-16T13:30:52Z

src/matrix.jl

+      return false  # obviously not unimodular
+    end
+    det_mod_m = det_mod_p
+    mul!(M, M, p)


Suggested change

mul!(M, M, p)

M = mul!(M, p)

fingolfin · 2025-01-16T13:31:46Z

src/matrix.jl

+# THIS FUNCTION NOT OFFICIALLY DOCUMENTED -- not intended for public use.
+function is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)


Instead of a code comment which nobody will see, I suggest we start using a prefix like _ to indicate internal functions:

Suggested change

# THIS FUNCTION NOT OFFICIALLY DOCUMENTED -- not intended for public use.

function is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)

function _is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)

Added new func is_unimodular for square integer matrices

aebe446

JohnAAbbott added 7 commits January 14, 2025 17:14

More test coverage

1992616

Fixed syntax error

564c282

Removed lots of semicolons (o/w someone will kick up a fuss)

2062d74

Minor revision to reduce code coverage complaints

f771c9d

Increased test coverage

9a8c48d

Increase test coverage

d62285c

Increase test coverage

f6c2d67

fieker merged commit f143dcb into Nemocas:master Jan 16, 2025
24 checks passed

JohnAAbbott deleted the JAA/is_unimodular branch January 16, 2025 10:34

fingolfin reviewed Jan 17, 2025

View reviewed changes

lgoettgens mentioned this pull request Jan 23, 2025

Soliciting design advice/opinions: speed-and-memory-hungry vs. slow-but-sure oscar-system/Oscar.jl#4442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new func is_unimodular for square integer matrices #1991

Added new func is_unimodular for square integer matrices #1991

JohnAAbbott commented Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

JohnAAbbott commented Jan 15, 2025

thofma commented Jan 16, 2025

JohnAAbbott commented Jan 16, 2025

thofma commented Jan 16, 2025

JohnAAbbott commented Jan 16, 2025

fieker commented Jan 16, 2025 via email

thofma commented Jan 16, 2025

fieker commented Jan 16, 2025 via email

fieker commented Jan 16, 2025 via email

fingolfin left a comment

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

fingolfin Jan 16, 2025

		# THIS FUNCTION NOT OFFICIALLY DOCUMENTED -- not intended for public use.
		function is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)

	# THIS FUNCTION NOT OFFICIALLY DOCUMENTED -- not intended for public use.
	function is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)
	function _is_unimodular_given_det_mod_m(A::ZZMatrix, det_mod_m::Int, M::ZZRingElem; algorithm=:auto)

Added new func is_unimodular for square integer matrices #1991

Added new func is_unimodular for square integer matrices #1991

Conversation

JohnAAbbott commented Jan 14, 2025

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

JohnAAbbott commented Jan 15, 2025

thofma commented Jan 16, 2025

JohnAAbbott commented Jan 16, 2025

thofma commented Jan 16, 2025

JohnAAbbott commented Jan 16, 2025

fieker commented Jan 16, 2025 via email

thofma commented Jan 16, 2025

fieker commented Jan 16, 2025 via email

fieker commented Jan 16, 2025 via email

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

fingolfin Jan 16, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 •

edited

Loading