From 443e7a4093a04ce9786abc905adc116e41124a02 Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Fri, 20 Sep 2024 22:24:13 +0200
Subject: [PATCH] Optimize is_zero_row/_column (#1802)

... by avoiding redundant bounds checks.

Also change the methods as well as that one for is_zero_entry to
apply to Julia matrices, too.
---
 src/Matrix.jl                | 18 ++++++++++--------
 src/julia/Matrix.jl          | 11 -----------
 test/generic/MatRing-test.jl |  5 +++++
 test/generic/Matrix-test.jl  |  5 +++++
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/Matrix.jl b/src/Matrix.jl
index 0aaccb95b6..205fbbd53e 100644
--- a/src/Matrix.jl
+++ b/src/Matrix.jl
@@ -233,20 +233,21 @@ function isone(a::MatrixElem{T}) where T <: NCRingElement
 end
 
 @doc raw"""
-    is_zero_entry(M::MatrixElem{T}, i::Int, j::Int) where T <: NCRingElement
+    is_zero_entry(M::Union{Matrix,MatrixElem}, i::Int, j::Int)
 
 Return `true` if $M_{i,j}$ is zero.
 """
-@inline is_zero_entry(M::MatrixElem{T}, i::Int, j::Int) where T <: NCRingElement = iszero(M[i,j])
+@inline is_zero_entry(M::Union{Matrix,MatrixElem}, i::Int, j::Int) = iszero(M[i,j])
 
 @doc raw"""
-    is_zero_row(M::MatrixElem{T}, i::Int) where T <: NCRingElement
+    is_zero_row(M::Union{Matrix,MatrixElem}, i::Int)
 
 Return `true` if the $i$-th row of the matrix $M$ is zero.
 """
-function is_zero_row(M::MatrixElem{T}, i::Int) where T <: NCRingElement
+function is_zero_row(M::Union{Matrix,MatrixElem}, i::Int)
+  @boundscheck 1 <= i <= nrows(M) || Base.throw_boundserror(M, (i, 1:ncols(M)))
   for j in 1:ncols(M)
-    if !is_zero_entry(M, i, j)
+    @inbounds if !is_zero_entry(M, i, j)
       return false
     end
   end
@@ -254,13 +255,14 @@ function is_zero_row(M::MatrixElem{T}, i::Int) where T <: NCRingElement
 end
 
 @doc raw"""
-    is_zero_column(M::MatrixElem{T}, j::Int) where T <: NCRingElement
+    is_zero_column(M::Union{Matrix,MatrixElem}, j::Int)
 
 Return `true` if the $j$-th column of the matrix $M$ is zero.
 """
-function is_zero_column(M::MatrixElem{T}, j::Int) where T <: NCRingElement
+function is_zero_column(M::Union{Matrix,MatrixElem}, j::Int)
+  @boundscheck 1 <= j <= ncols(M) || Base.throw_boundserror(M, (1:nrows(M), j))
   for i in 1:nrows(M)
-    if !is_zero_entry(M, i, j)
+    @inbounds if !is_zero_entry(M, i, j)
       return false
     end
   end
diff --git a/src/julia/Matrix.jl b/src/julia/Matrix.jl
index 5059b1086b..1577822dd0 100644
--- a/src/julia/Matrix.jl
+++ b/src/julia/Matrix.jl
@@ -7,17 +7,6 @@
 number_of_rows(A::Matrix{T}) where {T} = size(A)[1]
 number_of_columns(A::Matrix{T}) where {T} = size(A)[2]
 
-function is_zero_row(M::Matrix, i::Int)
-    for j = 1:ncols(M)
-        if !iszero(M[i, j])
-            return false
-        end
-    end
-    return true
-end
-
-# TODO: add `is_zero_column(M::Matrix{T}, i::Int) where {T<:Integer}` and specialized functions in Nemo
-
 zero_matrix(::Type{Int}, r, c) = zeros(Int, r, c)
 
 ###############################################################################
diff --git a/test/generic/MatRing-test.jl b/test/generic/MatRing-test.jl
index 88896ba261..cc726de152 100644
--- a/test/generic/MatRing-test.jl
+++ b/test/generic/MatRing-test.jl
@@ -125,6 +125,11 @@ end
    @test is_zero_column(C, 2)
    @test !is_zero_column(C, 1)
 
+   @test_throws BoundsError is_zero_row(C, 0)
+   @test_throws BoundsError is_zero_row(C, 4)
+   @test_throws BoundsError is_zero_column(C, 0)
+   @test_throws BoundsError is_zero_column(C, 4)
+
    S = matrix_ring(QQ, 3)
    A = S([1 2 3; 4 5 6; 7 8 9])
 
diff --git a/test/generic/Matrix-test.jl b/test/generic/Matrix-test.jl
index acde8e866a..bde5ebc7ae 100644
--- a/test/generic/Matrix-test.jl
+++ b/test/generic/Matrix-test.jl
@@ -542,6 +542,11 @@ end
    @test is_zero_column(C, 2)
    @test !is_zero_column(C, 1)
 
+   @test_throws BoundsError is_zero_row(C, 0)
+   @test_throws BoundsError is_zero_row(C, 4)
+   @test_throws BoundsError is_zero_column(C, 0)
+   @test_throws BoundsError is_zero_column(C, 4)
+
    @test length(A) == length(B) == length(C) == 9
    @test !any(isempty, (A, B, C))