sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

use libgap in simplicial_complex #34769

Closed dimpase closed 1 year ago

dimpase commented 1 year ago

this is just a 1-line change.

A part of #26902

CC: @jhpalmieri

Component: algebraic topology

Author: Dima Pasechnik

Branch/Commit: 1867190

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/34769

jhpalmieri commented 1 year ago
comment:2

Would it make sense to make it a 2-line change instead?

diff --git a/src/sage/topology/simplicial_complex.py b/src/sage/topology/simplicial_complex.py
index fe237895d8..857430769a 100644
--- a/src/sage/topology/simplicial_complex.py
+++ b/src/sage/topology/simplicial_complex.py
@@ -4081,7 +4081,7 @@ class SimplicialComplex(Parent, GenericCellComplex):
             return self.connected_component(Simplex([base_point])).fundamental_group(simplify=simplify)

         from sage.groups.free_group import FreeGroup
-        from sage.libs.gap.libgap import libgap as gap
+        from sage.libs.gap.libgap import libgap
         G = self.graph()
         # If the vertices and edges of G are not sortable, e.g., a mix
         # of str and int, Sage+Python 3 may raise a TypeError when
@@ -4098,7 +4098,7 @@ class SimplicialComplex(Parent, GenericCellComplex):
                 for e in G2.edges(sort=True)
                 if e not in spanning_tree]
         if len(gens) == 0:
-            return gap.TrivialGroup()
+            return libgap.TrivialGroup()

         # Edges in the graph may be sorted differently than in the
         # simplicial complex, so convert the edges to frozensets so we
dimpase commented 1 year ago
comment:3

We have imported libgap as gap, thus gap is the right thing in the scope, no?

jhpalmieri commented 1 year ago
comment:4

Your proposed change is this:

diff --git a/src/sage/topology/simplicial_complex.py b/src/sage/topology/simplicial_complex.py
index 736df96..fe23789 100644
--- a/src/sage/topology/simplicial_complex.py
+++ b/src/sage/topology/simplicial_complex.py
@@ -4081,7 +4081,7 @@ class SimplicialComplex(Parent, GenericCellComplex):
             return self.connected_component(Simplex([base_point])).fundamental_group(simplify=simplify)

         from sage.groups.free_group import FreeGroup
-        from sage.interfaces.gap import gap
+        from sage.libs.gap.libgap import libgap as gap
         G = self.graph()
         # If the vertices and edges of G are not sortable, e.g., a mix
         # of str and int, Sage+Python 3 may raise a TypeError when

I'm suggesting just doing from ... import libgap and also changing the one other line that uses gap.

jhpalmieri commented 1 year ago
comment:5

The Sage library tends to use from ... import libgap rather than from ... import libgap as gap, and consistency might be good.

dimpase commented 1 year ago
comment:6

as our aim is to drop pexpect gap, and rename libgap -> gap, I don't see why we should keep the convention you mentioned. It is less changes yo be done at the final stage this way (with your proposal,we will then have to change back all libgap. to gap.

jhpalmieri commented 1 year ago

Reviewer: John Palmieri

jhpalmieri commented 1 year ago
comment:7

It's fine, we can leave this as is, but currently searching for "import libgap as gap" (that is, git grep "import.*libgap.*as.*gap) comes up empty in the Sage library. I didn't know there was a proposal to rename libgap to gap.

vbraun commented 1 year ago

Changed branch from u/dimpase/topology/use_libgap to 1867190