From 811bb87742e99e495748d2bf24d14d7e689b0301 Mon Sep 17 00:00:00 2001
From: Tommy Hofmann <thofma@gmail.com>
Date: Tue, 5 Dec 2023 21:51:27 +0100
Subject: [PATCH] Fix some issues found by Markus Kirschmer (#1314)

---
 src/NumFieldOrd/NfOrd/FracIdeal.jl   | 18 ++++++++++--------
 src/NumFieldOrd/NfOrd/Ideal/Ideal.jl | 11 ++++-------
 test/NfOrd/Ideal.jl                  |  6 ++++++
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/NumFieldOrd/NfOrd/FracIdeal.jl b/src/NumFieldOrd/NfOrd/FracIdeal.jl
index d6e63b6429..d827d58438 100644
--- a/src/NumFieldOrd/NfOrd/FracIdeal.jl
+++ b/src/NumFieldOrd/NfOrd/FracIdeal.jl
@@ -52,7 +52,7 @@ end
 ################################################################################
 
 function iszero(x::NfAbsOrdFracIdl)
-  return iszero(numerator(x))
+  return iszero(numerator(x, copy = false))
 end
 
 #################################################################################
@@ -234,12 +234,14 @@ order(a::NfAbsOrdFracIdl) = a.order
 
 Returns the inverse of the basis matrix of $I$.
 """
-function basis_mat_inv(a::NfAbsOrdFracIdl)
-  if isdefined(a, :basis_mat_inv)
-    return deepcopy(a.basis_mat_inv)
-  else
+function basis_mat_inv(a::NfAbsOrdFracIdl; copy::Bool = true)
+  if !isdefined(a, :basis_mat_inv)
     a.basis_mat_inv = inv(basis_matrix(a))
+  end
+  if copy
     return deepcopy(a.basis_mat_inv)
+  else
+    return a.basis_mat_inv
   end
 end
 
@@ -627,7 +629,7 @@ function +(A::NfAbsOrdIdl{S, T}, B::NfAbsOrdFracIdl{S, T}) where {S <: NumField,
     return fractional_ideal(order(A), A)
   end
   n1 = A*denominator(B)
-  n = n1 + numerator(B)
+  n = n1 + numerator(B, copy = false)
   return n//denominator(B)
 end
 
@@ -645,7 +647,7 @@ function +(A::NfAbsOrdFracIdl{S, T}, B::Hecke.NfAbsOrdFracIdl{S, T}) where {S <:
   d = lcm(denominator(A), denominator(B))
   ma = div(d, denominator(A))
   mb = div(d, denominator(B))
-  return (numerator(A)*ma + numerator(B)*mb)//d
+  return (numerator(A, copy = false)*ma + numerator(B, copy = false)*mb)//d
 end
 
 function *(x::T, y::NfAbsOrd{S, T}) where {S <: NumField, T <: NumFieldElem}
@@ -792,7 +794,7 @@ function in(x::nf_elem, y::NfOrdFracIdl)
   M = zero_matrix(FlintZZ, 1, degree(O))
   t = FakeFmpqMat(M)
   elem_to_mat_row!(t.num, 1, t.den, x)
-  v = t*basis_mat_inv(O)
+  v = t*basis_mat_inv(O, copy = false)
   v = v*B
 
   return v.den == 1
diff --git a/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl b/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl
index e8769e9c8f..0f50168df8 100644
--- a/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl
+++ b/src/NumFieldOrd/NfOrd/Ideal/Ideal.jl
@@ -268,23 +268,20 @@ checked whether $M$ defines an ideal (expensive). If `M_in_hnf` is set, then it
 that $M$ is already in lower left HNF.
 """
 function ideal(O::NfAbsOrd, M::ZZMatrix; check::Bool = false, M_in_hnf::Bool = false)
-  !M_in_hnf ? x = _hnf(M, :lowerleft) : nothing #sub-optimal, but == relies on the basis being thus
-  #_trace_call(;print = true)
-  I = NfAbsOrdIdl(O, M)
+  x = !M_in_hnf ? _hnf(M, :lowerleft) : M #sub-optimal, but == relies on the basis being thus
+  I = NfAbsOrdIdl(O, x)
   # The compiler stopped liking this recursion??
   # if check
   #   J = ideal(O, basis(I))
   #   @assert J == I
   # end
-
   return I
 end
 
 function _ideal(O::NfAbsOrd, M::ZZMatrix, M_in_hnf::Bool = false)
-  !M_in_hnf ? x = _hnf(M, :lowerleft) : nothing #sub-optimal, but == relies on the basis being thus
+  x = !M_in_hnf ? _hnf(M, :lowerleft) : M #sub-optimal, but == relies on the basis being thus
   #_trace_call(;print = true)
-  I = NfAbsOrdIdl(O, M)
-
+  I = NfAbsOrdIdl(O, x)
   return I
 end
 
diff --git a/test/NfOrd/Ideal.jl b/test/NfOrd/Ideal.jl
index b4c85cc634..1ede5829e2 100644
--- a/test/NfOrd/Ideal.jl
+++ b/test/NfOrd/Ideal.jl
@@ -276,5 +276,11 @@
   Hecke.assure_2_normal(I)
   @test isdefined(I, :gen_two)
 
+  # Some issue with ideal(O, M)
+  K, a = quadratic_field(-1)
+  O = maximal_order(K)
+  @test basis_matrix(ideal(O, representation_matrix(O(a)))) == identity_matrix(ZZ, 2)
+  
   include("Ideal/Prime.jl")
+
 end