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: sage-7.6
Component: algebraic topology Keywords: days84, simplicial complex, star, stellar subdivision
Cc: moritz, vdelecroix Merged in:
Authors: Jean-Philippe Labbé Reviewers: Thierry Monteil, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 1b54fba (Commits, GitHub, GitLab) Commit: 1b54fba0f30ef57a4847fe96d771770782fcc612
Dependencies: Stopgaps:

Status badges

Description (last modified by jipilab)

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 jipilab

  • Branch set to u/jipilab/22466

comment:2 Changed 4 years ago by jipilab

  • 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

New commits:

2dfd035First implementation of star

comment:3 Changed 4 years ago by git

  • Commit changed from 2dfd035b6f09ec84dfbc5ebdb98809436e540091 to 04bcbefc61939ee3f2b7ced7542cc0cd3ca2c3cc

Branch pushed to git repo; I updated commit sha1. New commits:

04bcbefAdded first implementation of stellar subdivision

comment:4 Changed 4 years ago by git

  • Commit changed from 04bcbefc61939ee3f2b7ced7542cc0cd3ca2c3cc to e948253065efb36aae179b434f039429fcbf1c87

Branch pushed to git repo; I updated commit sha1. New commits:

c6b3853Corrected indentation
e948253pep8 conventions

comment:5 Changed 4 years ago by jipilab

  • Status changed from new to needs_review

comment:6 Changed 4 years ago by jipilab

  • Authors set to Jean-Philippe Labbé

comment:7 Changed 4 years ago by tmonteil

  • 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 non-working examples/tests to check such inconsistent user inputs.
  • in stellar_subdivision, the mutable option is not taken into account (does not appear in the code).
  • what is the difference between is_mutable and mutable option ?
  • you should add an INPUT section in stellar_subdivision.

comment:8 Changed 4 years ago by git

  • Commit changed from e948253065efb36aae179b434f039429fcbf1c87 to fae5016a6f594a40fd4757323059f49d9ca38af3

Branch pushed to git repo; I updated commit sha1. New commits:

fae5016Review and made tests pass

comment:9 Changed 4 years ago by chapoton

missing empty line here:

+        EXAMPLES::
+            sage: SC = SimplicialComplex([[0,1,2],[1,2,3]])

comment:10 Changed 4 years ago by git

  • Commit changed from fae5016a6f594a40fd4757323059f49d9ca38af3 to eb4dfac0e46749c72faada3d59d3b75b25e69662

Branch pushed to git repo; I updated commit sha1. New commits:

eb4dfacpep8 conventions

comment:11 Changed 4 years ago by git

  • Commit changed from eb4dfac0e46749c72faada3d59d3b75b25e69662 to 9ed526095a4f3b9e6ccbbaab4624ee5d7b9cd829

Branch pushed to git repo; I updated commit sha1. New commits:

9ed5260Corrected a silly typo

comment:12 follow-up: Changed 4 years ago by jhpalmieri

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

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:13 Changed 4 years ago by git

  • Commit changed from 9ed526095a4f3b9e6ccbbaab4624ee5d7b9cd829 to 2c1c6a84a1757778ede8aaa5d2fbfd6e93419a5a

Branch pushed to git repo; I updated commit sha1. New commits:

2c1c6a8pep8 conventions and fixed doc

comment:14 in reply to: ↑ 12 Changed 4 years ago by jipilab

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 jipilab

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 4 years ago by jhpalmieri

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): 
    27762776
    27772777        - ``simplex`` -- a simplex in this simplicial complex
    27782778        - ``is_mutable`` -- (default: ``True``) boolean; determines if the output
    2779             is mutable
     2779          is mutable
    27802780
    27812781        EXAMPLES::
    27822782

comment:17 Changed 4 years ago by git

  • Commit changed from 2c1c6a84a1757778ede8aaa5d2fbfd6e93419a5a to d68a1a2a9c7ced009bf659acb40f1662f66311b7

Branch pushed to git repo; I updated commit sha1. New commits:

d68a1a2Corrected indentation of input blocks

comment:18 in reply to: ↑ 16 Changed 4 years ago by jipilab

Oh! Okay, my bad! Should be okay now.

Thanks a lot for the help.

comment:19 Changed 4 years ago by chapoton

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 git

  • Commit changed from d68a1a2a9c7ced009bf659acb40f1662f66311b7 to 1b54fba0f30ef57a4847fe96d771770782fcc612

Branch pushed to git repo; I updated commit sha1. New commits:

1b54fbacorrected conventions

comment:21 Changed 4 years ago by chapoton

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

  • Branch changed from u/jipilab/22466 to 1b54fba0f30ef57a4847fe96d771770782fcc612
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.