Some fixes and performance improvements for `iso_oscar_gap(::LieAlgeb…

…ra)` (oscar-system#4227)

* Make `_iso_oscar_gap(::AbstractLieAlgeba)` way more efficient

* Move root system iso to its own function

* Some type stability

* Remove some unwanted iso caching

experimental/LieAlgebras/src/iso_gap_oscar.jl

@@ -26,9 +26,7 @@ function _iso_gap_oscar_abstract_lie_algebra(
   LO = _abstract_lie_algebra_from_GAP(LG, coeffs_iso, s)
   finv, f = _iso_oscar_gap_lie_algebra_functions(LO, LG, inv(coeffs_iso))
 
-  iso = MapFromFunc(LG, LO, f, finv)
-  set_attribute!(LO, :iso_oscar_gap => inv(iso))
-  return iso
+  return MapFromFunc(LG, LO, f, finv)
 end
 
 function _iso_gap_oscar_linear_lie_algebra(
@@ -39,9 +37,7 @@ function _iso_gap_oscar_linear_lie_algebra(
   LO = _linear_lie_algebra_from_GAP(LG, coeffs_iso, s)
   finv, f = _iso_oscar_gap_lie_algebra_functions(LO, LG, inv(coeffs_iso))
 
-  iso = MapFromFunc(LG, LO, f, finv)
-  set_attribute!(LO, :iso_oscar_gap => inv(iso))
-  return iso
+  return MapFromFunc(LG, LO, f, finv)
 end
 
 function _abstract_lie_algebra_from_GAP(

experimental/LieAlgebras/src/iso_oscar_gap.jl

@@ -22,7 +22,7 @@ function _iso_oscar_gap_lie_algebra_functions(
   return (f, finv)
 end
 
-function _iso_oscar_gap(LO::LinearLieAlgebra)
+function _iso_oscar_gap(LO::LinearLieAlgebra; set_attributes::Bool=true)
   coeffs_iso = Oscar.iso_oscar_gap(coefficient_ring(LO))
   LG = GAP.Globals.LieAlgebra(
     codomain(coeffs_iso),
@@ -32,6 +32,12 @@ function _iso_oscar_gap(LO::LinearLieAlgebra)
 
   f, finv = _iso_oscar_gap_lie_algebra_functions(LO, LG, coeffs_iso)
 
+  if set_attributes && has_root_system(LO)
+    # we need to construct the root system in GAP as otherwise it may detect a different order of simple roots
+    RO = root_system(LO)
+    _iso_oscar_gap_set_root_system(LG, RO)
+  end
+
   return MapFromFunc(LO, LG, f, finv)
 end
 
@@ -41,52 +47,56 @@ function _iso_oscar_gap(LO::AbstractLieAlgebra; set_attributes::Bool=true)
     [
       [
         begin
-          pairs = filter(pair -> !iszero(last(pair)), collect(enumerate(_matrix(xi * xj))))
-          (map(first, pairs), GAP.Obj[coeffs_iso(c) for c in map(last, pairs)])
-        end for xj in basis(LO)
-      ] for xi in basis(LO)
+          pairs = collect(LO.struct_consts[i, j])
+          (first.(pairs), GAP.Obj[coeffs_iso(c) for c in last.(pairs)])
+        end for j in 1:dim(LO)
+      ] for i in 1:dim(LO)
     ]
     -1
     coeffs_iso(zero(coefficient_ring(LO)))
   ]
 
-  LG = GAP.Globals.LieAlgebraByStructureConstants(
-    codomain(coeffs_iso), GAP.Obj(sc_table_G; recursive=true)
+  LG = GAPWrap.LieAlgebraByStructureConstants(
+    codomain(coeffs_iso), GapObj(sc_table_G; recursive=true)
   )
 
   f, finv = _iso_oscar_gap_lie_algebra_functions(LO, LG, coeffs_iso)
 
   if set_attributes && has_root_system(LO)
     # we need to construct the root system in GAP as otherwise it may detect a different order of simple roots
     RO = root_system(LO)
-    RG = GAP.Globals.Objectify(
-      GAP.Globals.NewType(
-        GAP.Globals.NewFamily(GAP.Obj("RootSystemFam"), GAP.Globals.IsObject),
-        GAP.evalstr("IsAttributeStoringRep and IsRootSystemFromLieAlgebra")),
-      GAP.GapObj(Dict{Symbol,Any}()))
-    GAP.Globals.SetUnderlyingLieAlgebra(RG, LG)
-
-    cartan_trO = transpose(cartan_matrix(RO))
-    transform_root(r::RootSpaceElem) = GAP.Obj(coefficients(r) * cartan_trO)[1]
-    GAP.Globals.SetPositiveRoots(RG, GAP.Obj(transform_root.(positive_roots(RO))))
-    GAP.Globals.SetNegativeRoots(RG, GAP.Obj(transform_root.(negative_roots(RO))))
-    GAP.Globals.SetSimpleSystem(RG, GAP.Obj(transform_root.(simple_roots(RO))))
-    can_basisG = GAP.Globals.CanonicalBasis(LG)
-    pos_root_vectorsG = can_basisG[1:n_positive_roots(RO)]
-    neg_root_vectorsG = can_basisG[(n_positive_roots(RO) + 1):(2 * n_positive_roots(RO))]
-    csa_basisG = can_basisG[(2 * n_positive_roots(RO) + 1):end]
-    GAP.Globals.SetPositiveRootVectors(RG, pos_root_vectorsG)
-    GAP.Globals.SetNegativeRootVectors(RG, neg_root_vectorsG)
-    GAP.Globals.SetCanonicalGenerators(
-      RG, GAP.Obj([pos_root_vectorsG, neg_root_vectorsG, csa_basisG])
-    )
-    GAP.Globals.SetChevalleyBasis(
-      LG, GAP.Obj([pos_root_vectorsG, neg_root_vectorsG, csa_basisG])
-    )
-
-    GAP.Globals.SetCartanMatrix(RG, GAP.Obj(cartan_trO))
-    GAP.Globals.SetRootSystem(LG, RG)
+    _iso_oscar_gap_set_root_system(LG, RO)
   end
 
   return MapFromFunc(LO, LG, f, finv)
 end
+
+function _iso_oscar_gap_set_root_system(LG::GapObj, RO::RootSystem)
+  RG = GAP.Globals.Objectify(
+    GAP.Globals.NewType(
+      GAP.Globals.NewFamily(GAP.Obj("RootSystemFam"), GAP.Globals.IsObject),
+      GAP.evalstr("IsAttributeStoringRep and IsRootSystemFromLieAlgebra")),
+    GAP.GapObj(Dict{Symbol,Any}()))
+  GAP.Globals.SetUnderlyingLieAlgebra(RG, LG)
+
+  cartan_trO = transpose(cartan_matrix(RO))
+  transform_root(r::RootSpaceElem) = GAP.Obj(coefficients(r) * cartan_trO)[1]
+  GAP.Globals.SetPositiveRoots(RG, GAP.Obj(transform_root.(positive_roots(RO))))
+  GAP.Globals.SetNegativeRoots(RG, GAP.Obj(transform_root.(negative_roots(RO))))
+  GAP.Globals.SetSimpleSystem(RG, GAP.Obj(transform_root.(simple_roots(RO))))
+  can_basisG = GAP.Globals.CanonicalBasis(LG)
+  pos_root_vectorsG = can_basisG[1:n_positive_roots(RO)]
+  neg_root_vectorsG = can_basisG[(n_positive_roots(RO) + 1):(2 * n_positive_roots(RO))]
+  csa_basisG = can_basisG[(2 * n_positive_roots(RO) + 1):end]
+  GAP.Globals.SetPositiveRootVectors(RG, pos_root_vectorsG)
+  GAP.Globals.SetNegativeRootVectors(RG, neg_root_vectorsG)
+  GAP.Globals.SetCanonicalGenerators(
+    RG, GAP.Obj([pos_root_vectorsG, neg_root_vectorsG, csa_basisG])
+  )
+  GAP.Globals.SetChevalleyBasis(
+    LG, GAP.Obj([pos_root_vectorsG, neg_root_vectorsG, csa_basisG])
+  )
+
+  GAP.Globals.SetCartanMatrix(RG, GAP.Obj(cartan_trO))
+  GAP.Globals.SetRootSystem(LG, RG)
+end

experimental/LieAlgebras/test/iso_gap_oscar-test.jl

@@ -6,8 +6,6 @@ function test_iso_gap_oscar(LG, oscarT; num_random_tests::Int=10)
   LO = codomain(iso)
   @test LO isa oscarT
 
-  @test codomain(Oscar.iso_oscar_gap(LO)) === LG
-
   @test iso === Oscar.iso_gap_oscar(LG) # test caching
 
   for _ in 1:num_random_tests

experimental/LieAlgebras/test/iso_oscar_gap-test.jl

@@ -5,7 +5,6 @@ function test_iso_oscar_gap(LO::LieAlgebra; num_random_tests::Int=10)
   @test domain(iso) === LO
   LG = codomain(iso)
   @test GAPWrap.IsLieAlgebra(LG)
-  # @test codomain(Oscar.iso_gap_oscar(LG)) === LO
 
   @test iso === Oscar.iso_oscar_gap(LO) # test caching
 

src/GAP/iso_oscar_gap.jl

@@ -402,7 +402,7 @@ function _iso_oscar_gap(FO::QQAbField)
 end
 
 """
-    Oscar.iso_oscar_gap(R) -> Map{T, GapObj}
+    Oscar.iso_oscar_gap(R::T) -> Map{T, GapObj}
 
 Return an isomorphism `f` with domain `R`
 and `codomain` a GAP object `S`.
@@ -481,7 +481,7 @@ true
     structure is not fully supported in GAP will likely cause overhead
     at runtime.
 """
-@attr Map function iso_oscar_gap(F)
+@attr Map{T, GapObj} function iso_oscar_gap(F::T) where T
    return _iso_oscar_gap(F)
 end
 

src/GAP/wrappers.jl

@@ -270,6 +270,7 @@ GAP.@wrap LargestMovedPoint(x::Any)::Int
 GAP.@wrap LeftActingDomain(x::GapObj)::GapObj
 GAP.@wrap LetterRepAssocWord(x::GapObj)::GapObj
 GAP.@wrap LibInfoCharacterTable(x::GapObj)::GapObj
+GAP.@wrap LieAlgebraByStructureConstants(x::GapObj, y::GapObj)::GapObj
 GAP.@wrap LinearCharacters(x::GapObj)::GapObj
 GAP.@wrap LinearCombination(x::GapObj, y::GapObj)::GapObj
 GAP.@wrap ListPerm(x::GapObj)::GapObj