Simplicial complexes: keep the __enlarged cache in add_face

[jhpalmieri]

The add_face method currently deletes the __enlarged cache, but this is unnecessary.

CC: @tscrim

Component: algebraic topology

Keywords: days74

Author: John Palmieri

Branch/Commit: c7bdbcf

Reviewer: Travis Scrimshaw

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

[jhpalmieri]

Commit: 1f18ca9

[jhpalmieri]

Branch: u/jhpalmieri/simplicial-add-face

[jhpalmieri]

comment:1

The _faces cache was also not treated properly, in both add_face and remove_face. For add_face, for example, you can see this if instead of the change here, you just delete the line self.__enlarged = {}: a doctest will fail because of an incorrect homology calculation, because of the wrong cached value for _faces.

---

New commits:

1f18ca9

Simplicial complexes: when adding a face, keep the __enlarged cache.

[jhpalmieri]

Changed keywords from none to days74

[tscrim]

comment:3

If you're okay with my changes, then you can set a positive review.

---

New commits:

00fce35

Reviewer changes to #20758.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

Changed branch from u/jhpalmieri/simplicial-add-face to u/tscrim/simplicial_add_face-20758

[tscrim]

Author: John Palmieri

[tscrim]

Changed commit from 1f18ca9 to 00fce35

[jhpalmieri]

comment:4

Looks good.

[vbraun]

comment:5

sage -t --long src/sage/homology/simplicial_complex.py
**********************************************************************
File "src/sage/homology/simplicial_complex.py", line 2606, in sage.homology.simplicial_complex.SimplicialComplex.remove_face
Failed example:
    T.remove_face((1,2,3))
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.homology.simplicial_complex.SimplicialComplex.remove_face[12]>", line 1, in <module>
        T.remove_face((Integer(1),Integer(2),Integer(3)))
      File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/homology/simplicial_complex.py", line 2659, in remove_face
        if L is None or Simplex(face) not in L:
      File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/homology/simplicial_complex.py", line 1204, in __contains__
        return x in self.faces()[dim]
    KeyError: 2
**********************************************************************
File "src/sage/homology/simplicial_complex.py", line 2607, in sage.homology.simplicial_complex.SimplicialComplex.remove_face
Failed example:
    len(T._faces)
Expected:
    2
Got:
    1
**********************************************************************
1 item had failures:
   2 of  17 in sage.homology.simplicial_complex.SimplicialComplex.remove_face
    [581 tests, 2 failures, 10.66 s]
sage -t --long src/sage/interfaces/interface.py

[jhpalmieri]

comment:6

This is fallout from #20718: in that ticket, we changed from using self.n_faces(dim) to using self.faces()[dim], but the former allows values of dim greater than the dimension of the complex: it just returns the empty set in that case. So we can continue with the philosophy in #20718, that we want to avoid n_faces, or we can restore its use in some cases. The fix is easy either way, we just need to decide what to do. For example, in keeping with the philosophy there:

diff --git a/src/sage/homology/simplicial_complex.py b/src/sage/homology/simplicial_complex.py
index fefc90e..1a6cca7 100644
--- a/src/sage/homology/simplicial_complex.py
+++ b/src/sage/homology/simplicial_complex.py
@@ -1199,7 +1199,7 @@ class SimplicialComplex(Parent, GenericCellComplex):
         if not isinstance(x, Simplex):
             return False
         dim = x.dimension()
-        return x in self.faces()[dim]
+        return dim in self.faces() and x in self.faces()[dim]
 
     def __call__(self, simplex):
         """
@@ -1527,9 +1527,11 @@ class SimplicialComplex(Parent, GenericCellComplex):
             ...
             ValueError: this simplex is not in this simplicial complex
         """
-        if not simplex in self.faces()[simplex.dimension()]:
+        d = simplex.dimension()
+        if d in self.faces() and simplex in self.faces()[d]:
+            return simplex.face(i)
+        else:
             raise ValueError('this simplex is not in this simplicial complex')
-        return simplex.face(i)
 
     def flip_graph(self):
         """
@@ -3220,7 +3222,11 @@ class SimplicialComplex(Parent, GenericCellComplex):
             sage: X.set_immutable()
             sage: X.n_skeleton(2)
             Simplicial complex with vertex set (0, 1, 2, 3) and facets {(0, 2, 3), (1, 2, 3), (0, 1)}
+            sage: X.n_skeleton(4)
+            Simplicial complex with vertex set (0, 1, 2, 3) and facets {(0, 2, 3), (1, 2, 3), (0, 1)}
         """
+        if n >= self.dimension():
+            return self
         # make sure it's a list (it will be a tuple if immutable)
         facets = [f for f in self._facets if f.dimension() < n]
         facets.extend(self.faces()[n])

[tscrim]

comment:7

I would say let us use self.faces()[dim] to continue to avoid using n_faces (the more we scrub it from the code, it is less likely that anyone will use it in the future [in the code]).

[jhpalmieri]

Changed branch from u/tscrim/simplicial_add_face-20758 to u/jhpalmieri/simplicial_add_face-20758

[jhpalmieri]

New commits:

15e54b1	trac 20718: use n_cells, not n_faces, and sort n_cells.
6bd5a97	grammar fix, capitalization
7b1a299	Having vertices() return a tuple instead of a Simplex.
3ace10e	Fixing a doctest in combinat/cluster_complex.py.
00c48e8	trac 20720: referee changes
954ef51	Merge branch 't/20720/public/simplicial_complex/vertices_output_type-20720' into homology_sorted
617a76a	trac 20718: fix issues with sorting (or not) of vertices
0b0a85d	Merge branch 't/20718/617a76ac2519d8219ee2640c26b0e1207642c5cb' into t/20758/simplicial_add_face-20758
c7bdbcf	simplicial complexes: a few more fixes with n_faces vs. faces

[jhpalmieri]

Changed commit from 00fce35 to c7bdbcf

[vbraun]

comment:10

sage -t --long src/sage/homology/simplicial_complex.py
**********************************************************************
File "src/sage/homology/simplicial_complex.py", line 2606, in sage.homology.simplicial_complex.SimplicialComplex.remove_face
Failed example:
    T.remove_face((1,2,3))
Exception raised:
    Traceback (most recent call last):
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.homology.simplicial_complex.SimplicialComplex.remove_face[12]>", line 1, in <module>
        T.remove_face((Integer(1),Integer(2),Integer(3)))
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/homology/simplicial_complex.py", line 2659, in remove_face
        if L is None or Simplex(face) not in L:
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/homology/simplicial_complex.py", line 1204, in __contains__
        return x in self.faces()[dim]
    KeyError: 2
**********************************************************************
File "src/sage/homology/simplicial_complex.py", line 2607, in sage.homology.simplicial_complex.SimplicialComplex.remove_face
Failed example:
    len(T._faces)
Expected:
    2
Got:
    1
**********************************************************************
1 item had failures:
   2 of  17 in sage.homology.simplicial_complex.SimplicialComplex.remove_face
    [581 tests, 2 failures, 6.66 s]

[jhpalmieri]

comment:11

I don't understand this error. With my branch, there is no line in the entire Sage source tree matching return x in self.faces()[dim]. The most recent changes one line (and I think the relevant line, from the __contains__ method) to

return dim in self.faces() and x in self.faces()[dim]

so this error should not happen.

All tests pass for me. I started over, checking out the branch all over again, and all tests still pass.

[vbraun]

comment:12

Well you wrote that in #20718 ;-)

just wait for the next beta and merge it in, easy.

[jhpalmieri]

comment:13

Replying to @vbraun:

Well you wrote that in #20718 ;-)

Right, and then changed it in the most recent commit on this ticket (c7bdbcf).

[vbraun]

comment:14

Which is why you shouldn't switch positively reviewed tickets back and forth...

[jhpalmieri]

comment:15

I'm really confused now. You posted a doctest failure here, so I thought it was basically required to undo the positive review. Should I have left it as a positive review?

[vbraun]

comment:16

If its fixed then set it back to positive review.

If you want to avoid the confusion then don't go back after positive review. If it doesn't work then I'll tell you. Now you merged in #20718 but there is no guarantee that that ticket will even be in the next beta. Its likely but it might still get unmerged.

[vbraun]

comment:17

Just to be clear: I tested the branch from back when it was "positive review".

[vbraun]

Changed branch from u/jhpalmieri/simplicial_add_face-20758 to c7bdbcf