Python 3: minor fixes for simplicial_complex.py

[jhpalmieri]

This fixes a few of the Python 3-related doctest failures in simplicial_complex.py.

CC: @fchapoton @tscrim

Component: python3

Author: John Palmieri

Branch: dc6b7c9

Reviewer: Travis Scrimshaw

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

[jhpalmieri]

Branch: u/jhpalmieri/simplicial-py3

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Commit: bd76f58

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bd76f58

trac 25932: fix a few Python 3 issues in simplical_complex.py

[jhpalmieri]

comment:4

The change to AbelianGroup_class

diff --git a/src/sage/groups/abelian_gps/abelian_group.py b/src/sage/groups/abelian_gps/abelian_group.py
index e19199b949..bf6fe5dc2c 100644
--- a/src/sage/groups/abelian_gps/abelian_group.py
+++ b/src/sage/groups/abelian_gps/abelian_group.py
@@ -553,6 +553,8 @@ class AbelianGroup_class(UniqueRepresentation, AbelianGroupBase):
             cat = cat.Infinite()
         AbelianGroupBase.__init__(self, category=cat)

+    __hash__ = UniqueRepresentation.__hash__
+
     def is_isomorphic(left, right):
         """
         Check whether ``left`` and ``right`` are isomorphic

fixes a lot of other problems in src/sage/homology/, but it belongs on another ticket, probably part of some larger effort to fix hashing.

[jhpalmieri]

comment:5

Similarly with

diff --git a/src/sage/groups/perm_gps/permgroup_named.py b/src/sage/groups/perm_gps/permgroup_named.py
index da1774e96c..54fa034b4a 100644
--- a/src/sage/groups/perm_gps/permgroup_named.py
+++ b/src/sage/groups/perm_gps/permgroup_named.py
@@ -149,6 +149,8 @@ class PermutationGroup_unique(CachedRepresentation, PermutationGroup_generic):
         """
         return super(CachedRepresentation, self).__eq__(other)

+    __hash__ = CachedRepresentation.__hash__
+

 class PermutationGroup_symalt(PermutationGroup_unique):
     """

Many of the remaining failures are I think due to sorting differences between Python 2 and Python 3.

[jhpalmieri]

comment:6

I guess these are covered by #24551. I'll add a comment there.

[tscrim]

comment:8

Some comments:

It seems excessive to first construct a list/tuple, which is then sorted:

         try:
-            return sorted(tuple(set(self))) < sorted(tuple(set(other)))
+            return sorted(self) < sorted(other)
         except TypeError:
-            return sorted([str(_) for _ in self]) < sorted([str(_) for _ in other])
+            return sorted(map(str,self)) < sorted(map(str, other))

(or you could just remove the list part of the latter). Actually, you may just want to explicitly call self.__set here too.

In the test for face_iterator, instead of # random, why not just sort the output (this makes the test more robust at detecting failures). Same for n_cells.

In the _repr_, why not sort the vertices by str for consistent output on Python3?

Beyond those, LGTM.

[tscrim]

Reviewer: Travis Scrimshaw

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from bd76f58 to 1f5f05f

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

1f5f05f

trac 25932: some clean up

[jhpalmieri]

comment:10

Thanks for the comments. By the way, I also couldn't decided whether to make this change in the __init__ method for simplicial complexes:

diff --git a/src/sage/homology/simplicial_complex.py b/src/sage/homology/simplicial_complex.py
index 7c5f7373d0..a29539b634 100644
--- a/src/sage/homology/simplicial_complex.py
+++ b/src/sage/homology/simplicial_complex.py
@@ -1018,7 +1018,10 @@ class SimplicialComplex(Parent, GenericCellComplex):
         if isinstance(vertex_set, (int, Integer)):
             vertices = tuple(range(vertex_set + 1))
         elif sort_facets:
-            vertices = tuple(sorted(vertex_set))
+            try:
+                vertices = tuple(sorted(vertex_set))
+            except TypeError:
+                vertices = tuple(sorted(vertex_set, key=str))
         else:
             vertices = tuple(vertex_set)
         gen_dict = {}
@@ -1049,7 +1052,10 @@ class SimplicialComplex(Parent, GenericCellComplex):
                 any(face.is_face(other) for other in good_faces)):
                 continue
             if sort_facets:
-                face = Simplex(sorted(face.tuple()))
+                try:
+                    face = Simplex(sorted(face.tuple()))
+                except TypeError:
+                    face = Simplex(sorted(face.tuple(), key=str))
             good_faces.append(face)

         # if no maximal faces, add the empty face as a facet

I have not done this, but some simplicial complex constructions mix integers and strings for vertex names while being defined with sort_facets=True.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

16e3aa2

trac 25932: one more fix so that sorting works with Python 3

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 1f5f05f to 16e3aa2

[tscrim]

comment:14

We might as well make it fully work in Python3 as without the change in comment:10, I would suspect we could get (unnecessary) failures.

I am starting to wonder how much we really need the vertices to be a tuple and not a set. Is there some reason for this? I feel like all of this extra sorting is making things more difficult to maintain than it should be.

If you want to address that in a later ticket, you can go ahead and set this to a positive review once the changes from comment:10 are incorporated.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

dc6b7c9

trac 25932: if sort_facets is True, sorting should always succeed,

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 16e3aa2 to dc6b7c9

[jhpalmieri]

comment:16

I'm working on a separate set of changes for sorting in the homology directory, so I think I'll put off any further changes to there. We absolutely need sorting (on faces and therefore, I think, on vertices) when we compute chain complexes and homology: various parts of that code need consistently ordered lists of n-simplices, for example.

If that is the only place where sorting is really needed, maybe we can get rid of it elsewhere. I don't know if that's a good idea yet or not. I'll think about it.

[jhpalmieri]

comment:18

Replying to @tscrim:

I am starting to wonder how much we really need the vertices to be a tuple and not a set. Is there some reason for this? I feel like all of this extra sorting is making things more difficult to maintain than it should be.

I've started thinking about this.

First, when I first implemented simplicial complexes, I wanted to view the vertices as forming a simplex, in which case the order matters. (I think it is important that, given a simplex, its nth face is well defined.) That's now how it's actually implemented – self._vertex_set is just a tuple – but that was part of the original philosphy.

Next, and more practically, sorting individual faces is necessary in a number of places, so doing it once, centrally, makes sense to me. I tried disabling the sort_facets argument: so that setting it has no effect, and lots of things broke. I might have to put sorting separately into graph, fundamental_group, chain_complex, and the method that converts simplicial complexes to simplicial sets. Spreading the sorting out makes it more likely that two different methods will use incompatible sortings, which will break things.

So if we want to go this route, we would need to have a new method which returns a set/list/iterable of some type of sorted faces, and then always use that. I've tried doing that, and I'm running into problems. I don't know if it's worth continuing.

[tscrim]

comment:19

If there is a need for the sorting, which I agree that is necessary for some things, I concur that it is easier to do in a central location. This outweighs any construction speedups that we might get for forgetting the sorting as most of the computational aspects of the problem are going to be after constructing a simplicial complex (e.g., finding its cohomology). Thank you for thinking about it and your explanations.

[vbraun]

Changed branch from u/jhpalmieri/simplicial-py3 to dc6b7c9

[jdemeyer]

comment:21

I don't like this solution:

            try:
                vertices = tuple(sorted(vertex_set))
            except TypeError:
                vertices = tuple(sorted(vertex_set, key=str))

See #26931

[jdemeyer]

Changed commit from dc6b7c9 to none