Added functions to get HNF transformation matrices by jstriete · Pull Request #1879 · thofma/Hecke.jl

jstriete · 2025-06-05T04:27:35Z

Edits HNF.jl and UpperTriangular.jl to add some functions that will return requested transformation matrices for the HNF decomposition.

thofma

Thanks! I left a few minor comments. Apart from these things and adding tests, this looks good.

thofma · 2025-06-05T04:37:54Z

src/Sparse/HNF.jl

 Like a snf without the divisibility condition.
 """
-function diagonal_form(A::SMat{ZZRingElem})
+function diagonal_form(A::SMat{ZZRingElem}, with_right_trafo = false, with_left_trafo = false)


I guess this is not used anymore, right?

thofma · 2025-06-05T04:38:36Z

src/Sparse/UpperTriangular.jl

 #
 ################################################################################

+function diagonal_form_with_transform(A::SMat, left=false, right=false)


Can you add a docstring or comment in the code?

thofma · 2025-06-05T04:40:06Z

src/Sparse/UpperTriangular.jl

+        end
+        hnf_with_trafo!(A, I, truncate=false, full_hnf=true, auto=false)
+      	R = sparse_matrix(base_ring(C), nrows(I), ncols(C))
+        for (i, row) in enumerate(I.rows)


Suggested change

for (i, row) in enumerate(I.rows)

for (i, row) in enumerate(I)

thofma · 2025-06-05T04:40:22Z

src/Sparse/UpperTriangular.jl

+      if left == false
+        hnf!(A, truncate=false, full_hnf=true, auto=false)
+      else
+ 	I = sparse_matrix(ZZ, 0, nrows(A))


Suggested change

I = sparse_matrix(ZZ, 0, nrows(A))

I = sparse_matrix(ZZ, 0, nrows(A))

thofma · 2025-06-05T04:40:45Z

src/Sparse/UpperTriangular.jl

+        end
+        hnf_with_trafo!(A, I, truncate=false, full_hnf=true, auto=false)
+        R = sparse_matrix(base_ring(D), nrows(I), ncols(D))
+        for (i, row) in enumerate(I.rows)


Suggested change

for (i, row) in enumerate(I.rows)

for (i, row) in enumerate(I)

thofma · 2025-06-05T04:41:38Z

src/Sparse/UpperTriangular.jl

+  if n % 2 == 0
+    A = transpose(A)
+  end
+  if left == true && right == true


Suggested change

if left == true && right == true

if left && right

thofma · 2025-06-05T04:41:54Z

src/Sparse/UpperTriangular.jl

+  if left == true && right == true
+    return C, A, transpose(D)
+  end
+  if left == true


Suggested change

if left == true

if left

thofma · 2025-06-05T04:42:06Z

src/Sparse/UpperTriangular.jl

+  if left == true
+    return C, A
+  end
+  if right == true


Suggested change

if right == true

if right

thofma · 2025-06-05T04:42:20Z

src/Sparse/UpperTriangular.jl


+function diagonal_form_with_transform(A::SMat, left=false, right=false)
+  n = 1
+  if left == true


Suggested change

if left == true

if left

thofma · 2025-06-05T04:42:35Z

src/Sparse/UpperTriangular.jl

+      push!(C, sparse_row(ZZ, [(i, ZZ(1))]))
+    end
+  end
+  if right == true


Suggested change

if right == true

if right

codecov · 2025-06-05T05:43:28Z

Codecov Report

Attention: Patch coverage is 7.31707% with 76 lines in your changes missing coverage. Please review.

Project coverage is 76.46%. Comparing base (d0f456f) to head (12820f7).

Files with missing lines	Patch %	Lines
src/Sparse/UpperTriangular.jl	0.00%	54 Missing ⚠️
src/Sparse/HNF.jl	21.42%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1879      +/-   ##
==========================================
- Coverage   76.54%   76.46%   -0.08%     
==========================================
  Files         364      364              
  Lines      115639   115720      +81     
==========================================
- Hits        88513    88489      -24     
- Misses      27126    27231     +105

Files with missing lines	Coverage Δ
src/Sparse/HNF.jl	`69.44% <21.42%> (-2.57%)`	⬇️
src/Sparse/UpperTriangular.jl	`18.94% <0.00%> (-24.96%)`	⬇️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fingolfin · 2025-06-11T00:34:31Z

src/Sparse/UpperTriangular.jl

+    C = sparse_matrix(ZZ, 0, nrows(A))
+    for i in 1:nrows(A)
+      push!(C, sparse_row(ZZ, [(i, ZZ(1))]))
+    end


Suggested change

C = sparse_matrix(ZZ, 0, nrows(A))

for i in 1:nrows(A)

push!(C, sparse_row(ZZ, [(i, ZZ(1))]))

end

C = identity_matrix(SMat, ZZ, nrows(A))

fingolfin · 2025-06-11T00:34:52Z

src/Sparse/UpperTriangular.jl

+    D = sparse_matrix(ZZ, 0, ncols(A))
+    for i in 1:ncols(A)
+      push!(D, sparse_row(ZZ, [(i, ZZ(1))]))
+    end


Suggested change

D = sparse_matrix(ZZ, 0, ncols(A))

for i in 1:ncols(A)

push!(D, sparse_row(ZZ, [(i, ZZ(1))]))

end

D = identity_matrix(SMat, ZZ, ncols(A))

fingolfin · 2025-06-11T00:35:53Z

src/Sparse/UpperTriangular.jl

+      push!(D, sparse_row(ZZ, [(i, ZZ(1))]))
+    end
+  end
+  while(!is_diagonal(A))


Suggested change

while(!is_diagonal(A))

while !is_diagonal(A)

fingolfin · 2025-06-11T00:36:43Z

src/Sparse/UpperTriangular.jl

+ 	I = sparse_matrix(ZZ, 0, nrows(A))
+        for i in 1:nrows(A)
+          push!(I, sparse_row(ZZ, [(i, ZZ(1))]))
+        end


Suggested change

I = sparse_matrix(ZZ, 0, nrows(A))

for i in 1:nrows(A)

push!(I, sparse_row(ZZ, [(i, ZZ(1))]))

end

I = identity_matrix(SMat, ZZ, nrows(A))

fingolfin · 2025-06-11T00:37:33Z

src/Sparse/UpperTriangular.jl

+        I = sparse_matrix(ZZ, 0, ncols(A))
+        for i in 1:ncols(A)
+          push!(I, sparse_row(ZZ, [(i, ZZ(1))]))
+        end


Suggested change

I = sparse_matrix(ZZ, 0, ncols(A))

for i in 1:ncols(A)

push!(I, sparse_row(ZZ, [(i, ZZ(1))]))

end

I = identity_matrix(SMat, ZZ, ncols(A))

Added functions to get HNF transformation matrices

12820f7

thofma reviewed Jun 5, 2025

View reviewed changes

fingolfin reviewed Jun 11, 2025

View reviewed changes

	for (i, row) in enumerate(I.rows)
	for (i, row) in enumerate(I)

	I = sparse_matrix(ZZ, 0, nrows(A))
	I = sparse_matrix(ZZ, 0, nrows(A))

Conversation

jstriete commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thofma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 5, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jstriete commented Jun 5, 2025 •

edited

Loading