Opened 7 years ago

Closed 7 years ago

#13244 closed enhancement (fixed)

make some simplicial complexes faster

Reported by: jhpalmieri Owned by: jhpalmieri
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:

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 jhpalmieri 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by jhpalmieri

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by 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 7 years ago by tscrim

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. Perhaps that should be another ticket and this one be set to positive review?

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.

Last edited 7 years ago by tscrim (previous) (diff)

Changed 7 years ago by jhpalmieri

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

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 7 years ago by tscrim

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 7 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:7 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:8 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.