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 a good practice to look at the html documentation to make sure it is okay.

Version 0, edited 4 years ago by jhpalmieri (next)

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.