Opened 10 years ago

Closed 10 years ago

#13244 closed enhancement (fixed)

make some simplicial complexes faster

Reported by: John Palmieri Owned by: John Palmieri
Priority: minor Milestone: sage-5.5
Component: algebraic topology Keywords: sd40 simplicial
Cc: Merged in: sage-5.5.beta0
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

A few simplicial complexes in the file sage/homology/examples.py are defined by computing orbits of a some lists under the action of a specific group. This can take a second or two, so we can save time by doing the computations once and then just using the explicitly computed orbits after that. The attached patch does this: it moves the G-orbit code out of the methods for the appropriate simplicial complexes, replacing it with an explicit list of facets. It stores the G-orbit code as a stand-alone function, in case anyone wants to see how the facets were constructed in the first place.

This also adds a minimal triangulation of the Klein bottle, and it uses proper reST formatting for references.

Attachments (1)

trac_13244-simplicial.patch (30.4 KB) - added by John Palmieri 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by John Palmieri

Status: newneeds_review

comment:2 Changed 10 years ago by Frédéric Chapoton

The patch looks good, even if I am not quite convinced by the trading of time-consumption versus dataspace-usage.

But where is the patchbot ?

comment:3 in reply to:  2 Changed 10 years ago by Travis Scrimshaw

Replying to chapoton:

The patch looks good, even if I am not quite convinced by the trading of time-consumption versus dataspace-usage.

My belief is that this is the better approach since memory/disk space is cheap, especially compared to CPU cycles since it takes seconds. However I would argue that each of these should be implemented as (immutable) singletons unless someone can think of a reason why you would need a true duplicate.

Also a minor technical thing, from looking at the patch file I believe the reference in SimplicialComplexExamples?() on line 1027 is missing the linking underscore.

Version 0, edited 10 years ago by Travis Scrimshaw (next)

Changed 10 years ago by John Palmieri

Attachment: trac_13244-simplicial.patch added

comment:4 Changed 10 years ago by John Palmieri

I added the missing underscore; thanks for catching that.

My belief is that this is the better approach since memory/disk space is cheap

I certainly agree with this, but I wrote the patch.

However I would argue that each of these should be implemented as (immutable) singletons

I'm not sure what you mean by this. Do you mean that any two instances of simplicial_complexes.K3Surface() should be identical as far as Python is concerned? We could try making SimplicialComplex inherit from UniqueRepresentation, for example, or maybe we could do this for SimplicialComplexExamples. But I think it could be put off to another ticket.

comment:5 in reply to:  4 Changed 10 years ago by Travis Scrimshaw

Replying to jhpalmieri:

However I would argue that each of these should be implemented as (immutable) singletons

I'm not sure what you mean by this. Do you mean that any two instances of simplicial_complexes.K3Surface() should be identical as far as Python is concerned? We could try making SimplicialComplex inherit from UniqueRepresentation, for example, or maybe we could do this for SimplicialComplexExamples. But I think it could be put off to another ticket.

I think so. I think sage's @cached_method would take care of this...or perhaps something like

def an_example(self):
   if not self.has_attr(_an_example_output):
      self._an_example_output = SimplicialComplex(facets)
   return self._an_example_output

But I'm happy moving this to a later ticket.

comment:6 Changed 10 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Everything is looks good to me. I've created a ticket for the above #13566.

comment:7 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.4sage-5.5

comment:8 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.5.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.