Opened 18 months ago
Closed 18 months ago
#20758 closed enhancement (fixed)
Simplicial complexes: keep the __enlarged cache in add_face
Reported by:  jhpalmieri  Owned by:  

Priority:  minor  Milestone:  sage7.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 )
The add_face
method currently deletes the __enlarged
cache, but this is unnecessary.
Change History (19)
comment:1 Changed 18 months ago by
 Branch set to u/jhpalmieri/simplicialaddface
 Commit set to 1f18ca90a8b1574cf2d8f1de6cbc2bdaa161c39a
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 18 months ago by
 Keywords days74 added
comment:3 Changed 18 months ago by
 Branch changed from u/jhpalmieri/simplicialaddface to u/tscrim/simplicial_add_face20758
 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:
00fce35  Reviewer changes to #20758.

comment:5 Changed 18 months ago by
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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/homology/simplicial_complex.py", line 2659, in remove_face if L is None or Simplex(face) not in L: File "/mnt/disk/home/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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
 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): 1199 1199 if not isinstance(x, Simplex): 1200 1200 return False 1201 1201 dim = x.dimension() 1202 return x in self.faces()[dim]1202 return dim in self.faces() and x in self.faces()[dim] 1203 1203 1204 1204 def __call__(self, simplex): 1205 1205 """ … … class SimplicialComplex(Parent, GenericCellComplex): 1527 1527 ... 1528 1528 ValueError: this simplex is not in this simplicial complex 1529 1529 """ 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: 1531 1534 raise ValueError('this simplex is not in this simplicial complex') 1532 return simplex.face(i)1533 1535 1534 1536 def flip_graph(self): 1535 1537 """ … … class SimplicialComplex(Parent, GenericCellComplex): 3220 3222 sage: X.set_immutable() 3221 3223 sage: X.n_skeleton(2) 3222 3224 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)} 3223 3227 """ 3228 if n >= self.dimension(): 3229 return self 3224 3230 # make sure it's a list (it will be a tuple if immutable) 3225 3231 facets = [f for f in self._facets if f.dimension() < n] 3226 3232 facets.extend(self.faces()[n])
comment:7 Changed 18 months ago by
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
 Branch changed from u/tscrim/simplicial_add_face20758 to u/jhpalmieri/simplicial_add_face20758
comment:9 Changed 18 months ago by
 Commit changed from 00fce35fba80180814ebf3557e27491ee15082d2 to c7bdbcf797859670d9bcb0ce29c7692ea0832f2d
 Status changed from needs_work to needs_review
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_type20720' 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_face20758

c7bdbcf  simplicial complexes: a few more fixes with n_faces vs. faces

comment:10 Changed 18 months ago by
 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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 498, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/homology/simplicial_complex.py", line 2659, in remove_face if L is None or Simplex(face) not in L: File "/Users/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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
 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 followup: ↓ 13 Changed 18 months ago by
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
comment:14 Changed 18 months ago by
Which is why you shouldn't switch positively reviewed tickets back and forth...
comment:15 Changed 18 months ago by
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
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
Just to be clear: I tested the branch from back when it was "positive review".
comment:18 Changed 18 months ago by
 Status changed from needs_review to positive_review
comment:19 Changed 18 months ago by
 Branch changed from u/jhpalmieri/simplicial_add_face20758 to c7bdbcf797859670d9bcb0ce29c7692ea0832f2d
 Resolution set to fixed
 Status changed from positive_review to closed
The
_faces
cache was also not treated properly, in bothadd_face
andremove_face
. Foradd_face
, for example, you can see this if instead of the change here, you just delete the lineself.__enlarged = {
}: a doctest will fail because of an incorrect homology calculation, because of the wrong cached value for_faces
.New commits:
Simplicial complexes: when adding a face, keep the __enlarged cache.