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 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