Opened 9 years ago
Closed 8 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)
Change History (9)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 8 years ago by
comment:3 in reply to: ↑ 2 Changed 8 years ago by
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.
Changed 8 years ago by
comment:4 follow-up: ↓ 5 Changed 8 years ago by
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 8 years ago by
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 makingSimplicialComplex
inherit fromUniqueRepresentation
, for example, or maybe we could do this forSimplicialComplexExamples
. 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 8 years ago by
- 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 8 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:8 Changed 8 years ago by
- Merged in set to sage-5.5.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
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 ?