#25932 closed defect (fixed)
Python 3: minor fixes for simplicial_complex.py
Reported by:  John Palmieri  Owned by:  

Priority:  minor  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  Frédéric Chapoton, Travis Scrimshaw  Merged in:  
Authors:  John Palmieri  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  dc6b7c9 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
This fixes a few of the Python 3related doctest failures in simplicial_complex.py.
Change History (21)
comment:1 Changed 4 years ago by
Branch:  → u/jhpalmieri/simplicialpy3 

comment:2 Changed 4 years ago by
Commit:  → bd76f58de96eec2d26381695bf22663a4606f81d 

comment:3 Changed 4 years ago by
Status:  new → needs_review 

comment:4 Changed 4 years ago by
The change to AbelianGroup_class

src/sage/groups/abelian_gps/abelian_group.py
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 b class AbelianGroup_class(UniqueRepresentation, AbelianGroupBase): 553 553 cat = cat.Infinite() 554 554 AbelianGroupBase.__init__(self, category=cat) 555 555 556 __hash__ = UniqueRepresentation.__hash__ 557 556 558 def is_isomorphic(left, right): 557 559 """ 558 560 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.
comment:5 Changed 4 years ago by
Similarly with

src/sage/groups/perm_gps/permgroup_named.py
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 b class PermutationGroup_unique(CachedRepresentation, PermutationGroup_generic): 149 149 """ 150 150 return super(CachedRepresentation, self).__eq__(other) 151 151 152 __hash__ = CachedRepresentation.__hash__ 153 152 154 153 155 class PermutationGroup_symalt(PermutationGroup_unique): 154 156 """
Many of the remaining failures are I think due to sorting differences between Python 2 and Python 3.
comment:7 Changed 4 years ago by
Cc:  Travis Scrimshaw added 

comment:8 Changed 4 years ago by
Reviewers:  → Travis Scrimshaw 

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.
comment:9 Changed 4 years ago by
Commit:  bd76f58de96eec2d26381695bf22663a4606f81d → 1f5f05f6bbd4d5e06a53ffcec6f71c977e1204a5 

Branch pushed to git repo; I updated commit sha1. New commits:
1f5f05f  trac 25932: some clean up

comment:10 Changed 4 years ago by
Thanks for the comments. By the way, I also couldn't decided whether to make this change in the __init__
method for simplicial complexes:

src/sage/homology/simplicial_complex.py
diff git a/src/sage/homology/simplicial_complex.py b/src/sage/homology/simplicial_complex.py index 7c5f7373d0..a29539b634 100644
a b class SimplicialComplex(Parent, GenericCellComplex): 1018 1018 if isinstance(vertex_set, (int, Integer)): 1019 1019 vertices = tuple(range(vertex_set + 1)) 1020 1020 elif sort_facets: 1021 vertices = tuple(sorted(vertex_set)) 1021 try: 1022 vertices = tuple(sorted(vertex_set)) 1023 except TypeError: 1024 vertices = tuple(sorted(vertex_set, key=str)) 1022 1025 else: 1023 1026 vertices = tuple(vertex_set) 1024 1027 gen_dict = {} … … class SimplicialComplex(Parent, GenericCellComplex): 1049 1052 any(face.is_face(other) for other in good_faces)): 1050 1053 continue 1051 1054 if sort_facets: 1052 face = Simplex(sorted(face.tuple())) 1055 try: 1056 face = Simplex(sorted(face.tuple())) 1057 except TypeError: 1058 face = Simplex(sorted(face.tuple(), key=str)) 1053 1059 good_faces.append(face) 1054 1060 1055 1061 # 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
.
comment:11 Changed 4 years ago by
Status:  needs_review → needs_work 

comment:12 Changed 4 years ago by
Commit:  1f5f05f6bbd4d5e06a53ffcec6f71c977e1204a5 → 16e3aa2a6c8c43353ca4fbfb19364d8cf5b51ce7 

Branch pushed to git repo; I updated commit sha1. New commits:
16e3aa2  trac 25932: one more fix so that sorting works with Python 3

comment:13 Changed 4 years ago by
Status:  needs_work → needs_review 

comment:14 followup: 18 Changed 4 years ago by
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.
comment:15 Changed 4 years ago by
Commit:  16e3aa2a6c8c43353ca4fbfb19364d8cf5b51ce7 → dc6b7c9fd116eff5b9c9b31d394512ed981068c2 

Branch pushed to git repo; I updated commit sha1. New commits:
dc6b7c9  trac 25932: if sort_facets is True, sorting should always succeed,

comment:16 Changed 4 years ago by
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 nsimplices, 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.
comment:17 Changed 4 years ago by
Status:  needs_review → positive_review 

comment:18 Changed 4 years ago by
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.
comment:19 Changed 4 years ago by
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.
comment:20 Changed 4 years ago by
Branch:  u/jhpalmieri/simplicialpy3 → dc6b7c9fd116eff5b9c9b31d394512ed981068c2 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:21 Changed 4 years ago by
Commit:  dc6b7c9fd116eff5b9c9b31d394512ed981068c2 

I don't like this solution:
try: vertices = tuple(sorted(vertex_set)) except TypeError: vertices = tuple(sorted(vertex_set, key=str))
See #26931
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
trac 25932: fix a few Python 3 issues in simplical_complex.py