Opened 4 years ago
Closed 4 years ago
#22466 closed enhancement (fixed)
Implement star and stellar subdivision of a face of simplicial complex
Reported by:  jipilab  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  algebraic topology  Keywords:  days84, simplicial complex, star, stellar subdivision 
Cc:  moritz, vdelecroix  Merged in:  
Authors:  JeanPhilippe Labbé  Reviewers:  Thierry Monteil, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  1b54fba (Commits, GitHub, GitLab)  Commit:  1b54fba0f30ef57a4847fe96d771770782fcc612 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket provides the star and the stellar subdivision of a face of a simplicial complex.
The star of a face is the union of the faces that contains that face. The star of the empty face is the whole complex.
Given a simplicial complex C, the stellar subdivision of a face f of C is a new simplicial complex obtained as the barycentric subdivision of the face with respect to its star.
Change History (22)
comment:1 Changed 4 years ago by
 Branch set to u/jipilab/22466
comment:2 Changed 4 years ago by
 Commit set to 2dfd035b6f09ec84dfbc5ebdb98809436e540091
 Description modified (diff)
 Keywords deletion removed
 Summary changed from Implement deletion, star and stellar subdivision of a face of simplicial complex to Implement star and stellar subdivision of a face of simplicial complex
comment:3 Changed 4 years ago by
 Commit changed from 2dfd035b6f09ec84dfbc5ebdb98809436e540091 to 04bcbefc61939ee3f2b7ced7542cc0cd3ca2c3cc
Branch pushed to git repo; I updated commit sha1. New commits:
04bcbef  Added first implementation of stellar subdivision

comment:4 Changed 4 years ago by
 Commit changed from 04bcbefc61939ee3f2b7ced7542cc0cd3ca2c3cc to e948253065efb36aae179b434f039429fcbf1c87
comment:5 Changed 4 years ago by
 Status changed from new to needs_review
comment:6 Changed 4 years ago by
comment:7 Changed 4 years ago by
 Reviewers set to Thierry Monteil
First few comments/questions:
 would it be pertinent to check that the provided simplex is actually a simplex in the considered simplicial complex ?
 You could provide nonworking examples/tests to check such inconsistent user inputs.
 in
stellar_subdivision
, themutable
option is not taken into account (does not appear in the code).  what is the difference between
is_mutable
andmutable
option ?  you should add an
INPUT
section instellar_subdivision
.
comment:8 Changed 4 years ago by
 Commit changed from e948253065efb36aae179b434f039429fcbf1c87 to fae5016a6f594a40fd4757323059f49d9ca38af3
Branch pushed to git repo; I updated commit sha1. New commits:
fae5016  Review and made tests pass

comment:9 Changed 4 years ago by
missing empty line here:
+ EXAMPLES:: + sage: SC = SimplicialComplex([[0,1,2],[1,2,3]])
comment:10 Changed 4 years ago by
 Commit changed from fae5016a6f594a40fd4757323059f49d9ca38af3 to eb4dfac0e46749c72faada3d59d3b75b25e69662
Branch pushed to git repo; I updated commit sha1. New commits:
eb4dfac  pep8 conventions

comment:11 Changed 4 years ago by
 Commit changed from eb4dfac0e46749c72faada3d59d3b75b25e69662 to 9ed526095a4f3b9e6ccbbaab4624ee5d7b9cd829
Branch pushed to git repo; I updated commit sha1. New commits:
9ed5260  Corrected a silly typo

comment:12 followup: ↓ 14 Changed 4 years ago by
 Status changed from needs_review to needs_work
You should avoid lines longer than 80 characters. I see one here:
 `is_mutable`  (optional) boolean, determines if the output is mutable, default ``True``
In a line like
def stellar_subdivision(self,simplex,inplace=False,is_mutable=True):
the style is to put a space after each comma.
In docstrings, use single backquotes or dollar signs – `F`
, $F$
– for math, but use double backquotes – ``simplex``
, ``is_mutable``
– for code.
In doctests, you need to put a double colon ::
before each indented line, not just a single colon. For example, you need two colons at the end of this line
The simplex to subdivide should be a face of self::
I'm assuming that you haven't actually looked at the built documentation or you would have realized that there were problems. It's a good practice to look at the html documentation to make sure it is okay.
comment:13 Changed 4 years ago by
 Commit changed from 9ed526095a4f3b9e6ccbbaab4624ee5d7b9cd829 to 2c1c6a84a1757778ede8aaa5d2fbfd6e93419a5a
Branch pushed to git repo; I updated commit sha1. New commits:
2c1c6a8  pep8 conventions and fixed doc

comment:14 in reply to: ↑ 12 Changed 4 years ago by
Dear jhpalmieri,
Thank you for the comments. Indeed, the double ticks slipped out of my mind for the code part.
I fixed the documentation, hopefully it should be okay now.
comment:15 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:16 followup: ↓ 18 Changed 4 years ago by
Okay, better, but now the indentation doesn't match in the input and output blocks. You need to make changes like

src/sage/homology/simplicial_complex.py
diff git a/src/sage/homology/simplicial_complex.py b/src/sage/homology/simplicial_complex.py index 2f2e997..0c77839 100644
a b class SimplicialComplex(Parent, GenericCellComplex): 2776 2776 2777 2777  ``simplex``  a simplex in this simplicial complex 2778 2778  ``is_mutable``  (default: ``True``) boolean; determines if the output 2779 2779 is mutable 2780 2780 2781 2781 EXAMPLES:: 2782 2782
comment:17 Changed 4 years ago by
 Commit changed from 2c1c6a84a1757778ede8aaa5d2fbfd6e93419a5a to d68a1a2a9c7ced009bf659acb40f1662f66311b7
Branch pushed to git repo; I updated commit sha1. New commits:
d68a1a2  Corrected indentation of input blocks

comment:18 in reply to: ↑ 16 Changed 4 years ago by
Oh! Okay, my bad! Should be okay now.
Thanks a lot for the help.
comment:19 Changed 4 years ago by
1) The first line of the 2 new methods should rather be
+ """ + Return the star of a simplex in this simplicial complex.
and
+ """ + Return the stellar subdivision of a simplex in this simplicial complex.
2) in the raise statements the convention is not to start with a capital and not to end with a dot, so something like
+ raise ValueError("this simplicial complex is not mutable")
and
+ raise ValueError("the face to subdivide is not a face of self")
and then change the doctests accordingly.
3) I would add "or on a copy" here:
+  ``inplace``  (default: ``False``) boolean; determines if the + operation is done on ``self`` or on a copy
Otherwise, this looks good.
comment:20 Changed 4 years ago by
 Commit changed from d68a1a2a9c7ced009bf659acb40f1662f66311b7 to 1b54fba0f30ef57a4847fe96d771770782fcc612
Branch pushed to git repo; I updated commit sha1. New commits:
1b54fba  corrected conventions

comment:21 Changed 4 years ago by
 Reviewers changed from Thierry Monteil to Thierry Monteil, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be
comment:22 Changed 4 years ago by
 Branch changed from u/jipilab/22466 to 1b54fba0f30ef57a4847fe96d771770782fcc612
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
First implementation of star