Opened 18 months ago

Closed 17 months ago

Last modified 13 months ago

#25932 closed defect (fixed)

Python 3: minor fixes for simplicial_complex.py

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-8.4
Component: python3 Keywords:
Cc: chapoton, tscrim Merged in:
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: dc6b7c9 (Commits) Commit:
Dependencies: Stopgaps:

Description

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

Change History (21)

comment:1 Changed 18 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/simplicial-py3

comment:2 Changed 18 months ago by git

  • Commit set to bd76f58de96eec2d26381695bf22663a4606f81d

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

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

comment:3 Changed 18 months ago by jhpalmieri

  • Status changed from new to needs_review

comment:4 Changed 18 months ago by jhpalmieri

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): 
    553553            cat = cat.Infinite()
    554554        AbelianGroupBase.__init__(self, category=cat)
    555555
     556    __hash__ = UniqueRepresentation.__hash__
     557
    556558    def is_isomorphic(left, right):
    557559        """
    558560        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 18 months ago by jhpalmieri

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): 
    149149        """
    150150        return super(CachedRepresentation, self).__eq__(other)
    151151
     152    __hash__ = CachedRepresentation.__hash__
     153
    152154
    153155class PermutationGroup_symalt(PermutationGroup_unique):
    154156    """

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

comment:6 Changed 18 months ago by jhpalmieri

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

comment:7 Changed 18 months ago by jhpalmieri

  • Cc tscrim added

comment:8 Changed 18 months ago by tscrim

  • Reviewers set to 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 18 months ago by git

  • Commit changed from bd76f58de96eec2d26381695bf22663a4606f81d to 1f5f05f6bbd4d5e06a53ffcec6f71c977e1204a5

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

1f5f05ftrac 25932: some clean up

comment:10 Changed 18 months ago by jhpalmieri

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): 
    10181018        if isinstance(vertex_set, (int, Integer)):
    10191019            vertices = tuple(range(vertex_set + 1))
    10201020        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))
    10221025        else:
    10231026            vertices = tuple(vertex_set)
    10241027        gen_dict = {}
    class SimplicialComplex(Parent, GenericCellComplex): 
    10491052                any(face.is_face(other) for other in good_faces)):
    10501053                continue
    10511054            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))
    10531059            good_faces.append(face)
    10541060
    10551061        # 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 18 months ago by jhpalmieri

  • Status changed from needs_review to needs_work

comment:12 Changed 18 months ago by git

  • Commit changed from 1f5f05f6bbd4d5e06a53ffcec6f71c977e1204a5 to 16e3aa2a6c8c43353ca4fbfb19364d8cf5b51ce7

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

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

comment:13 Changed 18 months ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:14 follow-up: Changed 18 months ago by tscrim

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.

Last edited 18 months ago by tscrim (previous) (diff)

comment:15 Changed 18 months ago by git

  • Commit changed from 16e3aa2a6c8c43353ca4fbfb19364d8cf5b51ce7 to dc6b7c9fd116eff5b9c9b31d394512ed981068c2

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

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

comment:16 Changed 18 months ago by jhpalmieri

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.

comment:17 Changed 18 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:18 in reply to: ↑ 14 Changed 18 months ago by jhpalmieri

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 18 months ago by tscrim

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 17 months ago by vbraun

  • Branch changed from u/jhpalmieri/simplicial-py3 to dc6b7c9fd116eff5b9c9b31d394512ed981068c2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 13 months ago by jdemeyer

  • Commit dc6b7c9fd116eff5b9c9b31d394512ed981068c2 deleted

I don't like this solution:

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

See #26931

Note: See TracTickets for help on using tickets.