Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make some simplicial complexes faster #13244

Closed
jhpalmieri opened this issue Jul 12, 2012 · 8 comments
Closed

make some simplicial complexes faster #13244

jhpalmieri opened this issue Jul 12, 2012 · 8 comments

Comments

@jhpalmieri
Copy link
Member

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.

Component: algebraic topology

Keywords: sd40 simplicial

Author: John Palmieri

Reviewer: Travis Scrimshaw

Merged: sage-5.5.beta0

Issue created by migration from https://trac.sagemath.org/ticket/13244

@fchapoton
Copy link
Contributor

comment:2

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 ?

@tscrim
Copy link
Collaborator

tscrim commented Oct 1, 2012

comment:3

Replying to @fchapoton:

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.

@jhpalmieri
Copy link
Member Author

Attachment: trac_13244-simplicial.patch.gz

@jhpalmieri
Copy link
Member Author

comment:4

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.

@tscrim
Copy link
Collaborator

tscrim commented Oct 1, 2012

comment:5

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.

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2012

comment:6

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

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2012

Reviewer: Travis Scrimshaw

@jdemeyer
Copy link

Merged: sage-5.5.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants