#20758 closed enhancement (fixed)

Simplicial complexes: keep the __enlarged cache in add_face

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-7.3
Component: algebraic topology Keywords: days74
Cc: tscrim Merged in:
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: c7bdbcf (Commits) Commit: c7bdbcf797859670d9bcb0ce29c7692ea0832f2d
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

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

Change History (19)

comment:1 Changed 18 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/simplicial-add-face
  • Commit set to 1f18ca90a8b1574cf2d8f1de6cbc2bdaa161c39a
  • Description modified (diff)
  • Status changed from new to needs_review

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:

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

comment:2 Changed 18 months ago by jhpalmieri

  • Keywords days74 added

comment:3 Changed 18 months ago by tscrim

  • Authors set to John Palmieri
  • Branch changed from u/jhpalmieri/simplicial-add-face to u/tscrim/simplicial_add_face-20758
  • Commit changed from 1f18ca90a8b1574cf2d8f1de6cbc2bdaa161c39a to 00fce35fba80180814ebf3557e27491ee15082d2
  • Reviewers set to Travis Scrimshaw

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


New commits:

00fce35Reviewer changes to #20758.

comment:4 Changed 18 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

Looks good.

comment:5 Changed 18 months ago by vbraun

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

comment:6 Changed 18 months ago by jhpalmieri

  • Status changed from positive_review to needs_work

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:

  • src/sage/homology/simplicial_complex.py

    diff --git a/src/sage/homology/simplicial_complex.py b/src/sage/homology/simplicial_complex.py
    index fefc90e..1a6cca7 100644
    a b class SimplicialComplex(Parent, GenericCellComplex): 
    11991199        if not isinstance(x, Simplex):
    12001200            return False
    12011201        dim = x.dimension()
    1202         return x in self.faces()[dim]
     1202        return dim in self.faces() and x in self.faces()[dim]
    12031203
    12041204    def __call__(self, simplex):
    12051205        """
    class SimplicialComplex(Parent, GenericCellComplex): 
    15271527            ...
    15281528            ValueError: this simplex is not in this simplicial complex
    15291529        """
    1530         if not simplex in self.faces()[simplex.dimension()]:
     1530        d = simplex.dimension()
     1531        if d in self.faces() and simplex in self.faces()[d]:
     1532            return simplex.face(i)
     1533        else:
    15311534            raise ValueError('this simplex is not in this simplicial complex')
    1532         return simplex.face(i)
    15331535
    15341536    def flip_graph(self):
    15351537        """
    class SimplicialComplex(Parent, GenericCellComplex): 
    32203222            sage: X.set_immutable()
    32213223            sage: X.n_skeleton(2)
    32223224            Simplicial complex with vertex set (0, 1, 2, 3) and facets {(0, 2, 3), (1, 2, 3), (0, 1)}
     3225            sage: X.n_skeleton(4)
     3226            Simplicial complex with vertex set (0, 1, 2, 3) and facets {(0, 2, 3), (1, 2, 3), (0, 1)}
    32233227        """
     3228        if n >= self.dimension():
     3229            return self
    32243230        # make sure it's a list (it will be a tuple if immutable)
    32253231        facets = [f for f in self._facets if f.dimension() < n]
    32263232        facets.extend(self.faces()[n])

comment:7 Changed 18 months ago by tscrim

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]).

comment:8 Changed 18 months ago by jhpalmieri

  • Branch changed from u/tscrim/simplicial_add_face-20758 to u/jhpalmieri/simplicial_add_face-20758

comment:9 Changed 18 months ago by jhpalmieri

  • Commit changed from 00fce35fba80180814ebf3557e27491ee15082d2 to c7bdbcf797859670d9bcb0ce29c7692ea0832f2d
  • Status changed from needs_work to needs_review

New commits:

15e54b1trac 20718: use n_cells, not n_faces, and sort n_cells.
6bd5a97grammar fix, capitalization
7b1a299Having vertices() return a tuple instead of a Simplex.
3ace10eFixing a doctest in combinat/cluster_complex.py.
00c48e8trac 20720: referee changes
954ef51Merge branch 't/20720/public/simplicial_complex/vertices_output_type-20720' into homology_sorted
617a76atrac 20718: fix issues with sorting (or not) of vertices
0b0a85dMerge branch 't/20718/617a76ac2519d8219ee2640c26b0e1207642c5cb' into t/20758/simplicial_add_face-20758
c7bdbcfsimplicial complexes: a few more fixes with n_faces vs. faces

comment:10 Changed 18 months ago by vbraun

  • Status changed from needs_review to needs_work
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]

comment:11 Changed 18 months ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

comment:12 follow-up: Changed 18 months ago by vbraun

Well you wrote that in #20718 ;-)

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

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

Replying to vbraun:

Well you wrote that in #20718 ;-)

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

comment:14 Changed 18 months ago by vbraun

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

comment:15 Changed 18 months ago by jhpalmieri

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?

comment:16 Changed 18 months ago by vbraun

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.

comment:17 Changed 18 months ago by vbraun

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

comment:18 Changed 18 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:19 Changed 18 months ago by vbraun

  • Branch changed from u/jhpalmieri/simplicial_add_face-20758 to c7bdbcf797859670d9bcb0ce29c7692ea0832f2d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.