Opened 8 years ago
Last modified 5 years ago
#13566 closed enhancement
Simplicial complex examples as singletons — at Version 34
Reported by:  tscrim  Owned by:  tscrim 

Priority:  minor  Milestone:  sage6.10 
Component:  algebraic topology  Keywords:  simplicial 
Cc:  jhpalmieri, vbraun  Merged in:  
Authors:  John Palmieri  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  u/jhpalmieri/simplicialexamples  Commit:  5bf74b6d7f517c534b668dc602b6c7138729156b 
Dependencies:  #13244, #12587, #14142  Stopgaps: 
Description (last modified by )
Since each of the examples are unique, there should only be one (immutable) instance of each. In other words, we do not recreate the example each time it is called. For example
sage: S1 = simplicial_complexes.KleinBottle() sage: S2 = simplicial_complexes.KleinBottle() sage: S1 == S2 True sage: S1 is S2 False
where the last should return true
. Possibly do this by something like
def KlienBottle(self): if not hasattr(self, "_klien_bottle_output"): self._klien_bottle_output = SimplicialComplex(facets) return self._klien_bottle_output
This is an expansion on the concept in #13244 and the dependency on #12587 is in making simplicial complexes immutable.
Change History (36)
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 Changed 8 years ago by
 Cc jhpalmieri added
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
 Status changed from new to needs_review
comment:5 Changed 8 years ago by
 Status changed from needs_review to needs_work
Please add doctests such as
sage: T1 = simplicial_complexes.Torus() sage: T2 = simplicial_complexes.Torus() sage: T1 is T2 True
and make sure this is based on #12587 (the ticket cannot apply with any fuzz). I'd also like to see you put your name in the AUTHORS:
block and a general statement of the changes. Also you will need to fill in your real name on the ticket and have a proper commit message in the patch.
Thank you,
Travis
comment:6 Changed 8 years ago by
Hmm, I've tried for about an hour to apply the patches from #12587 on my (essentially clean) 3.5.1, but without success. I think I'll just "withdraw" this patch for now and come back to it after the next release...
comment:7 followup: ↓ 8 Changed 8 years ago by
Sorry about that. I think I've recorded the correct dependencies on #12587 now.
comment:8 in reply to: ↑ 7 Changed 8 years ago by
Replying to jhpalmieri:
Sorry about that. I think I've recorded the correct dependencies on #12587 now.
Thanks, that might help. On the other hand, I don't think this issue is urgent so I've pushed it to the back of my queue for now. If someone else wants to implement a fix I wouldn't mind at all ;)
comment:9 Changed 8 years ago by
 Status changed from needs_work to needs_review
Here is a rebased version of Christian's patch. I added a bunch of tests, also.
Changed 8 years ago by
comment:10 Changed 8 years ago by
 Description modified (diff)
comment:11 Changed 8 years ago by
By the way, this applies independently of the patches at #13725 for sum complexes. Those can't be cached like this because they can accept a list (which is unhashable) as an argument.
comment:12 followup: ↓ 13 Changed 8 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
From what I've heard, custom caches are something we should avoid. So since we can't make sum complexes work with @cached_method
, and everything looks good to me, I'm setting this to positive review. Thanks.
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Replying to tscrim:
From what I've heard, custom caches are something we should avoid. So since we can't make sum complexes work with
@cached_method
, and everything looks good to me, I'm setting this to positive review. Thanks.
The author credits are very generous, many thanks. From my part this was a somewhat reckless case of driveby patching of which I am rather ashamed now. I hope to do a better job next time... ;)
Cheers,
Christian
comment:14 Changed 8 years ago by
 Milestone changed from sage5.5 to sage5.6
comment:15 Changed 8 years ago by
On sage.math, this patch causes
sage t force_lib devel/sage/sage/categories/algebra_modules.py /release/merger/sage5.6.beta2/local/lib/libcsage.so(print_backtrace+0x2b)[0x2b1002e4866e] /release/merger/sage5.6.beta2/local/lib/libcsage.so(sigdie+0x14)[0x2b1002e4869b] /release/merger/sage5.6.beta2/local/lib/libcsage.so(sage_signal_handler+0x20b)[0x2b1002e48189] /lib/libpthread.so.0[0x2b1000eb27d0] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyObject_GC_Del+0x22)[0x2b1000be54c2] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b62e01] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b31703] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b31978] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b31978] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000bd949b] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b44643] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyDict_SetItem+0x73)[0x2b1000b45583] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyDict_SetItemString+0x4b)[0x2b1000b4676b] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x47fd)[0x2b1000bab07d] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b329b9] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyObject_Call+0x68)[0x2b1000b05308] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b158bf] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyObject_Call+0x68)[0x2b1000b05308] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x1299)[0x2b1000ba7b19] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x5ae4)[0x2b1000bac364] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b329b9] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyObject_Call+0x68)[0x2b1000b05308] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0[0x2b1000b158bf] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyObject_Call+0x68)[0x2b1000b05308] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x1299)[0x2b1000ba7b19] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x5ae4)[0x2b1000bac364] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x5ae4)[0x2b1000bac364] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x5ae4)[0x2b1000bac364] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCodeEx+0x852)[0x2b1000bae352] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyEval_EvalCode+0x32)[0x2b1000bae472] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyRun_FileExFlags+0xc1)[0x2b1000bd21f1] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(PyRun_SimpleFileExFlags+0x1f9)[0x2b1000bd24c9] /release/merger/sage5.6.beta2/local/lib/libpython2.7.so.1.0(Py_Main+0xb15)[0x2b1000be5115] /lib/libc.so.6(__libc_start_main+0xf4)[0x2b10017671f4] python[0x400679]  Unhandled SIGSEGV: A segmentation fault occurred in Sage. This probably occurred because a *compiled* component of Sage has a bug in it and is not properly wrapped with sig_on(), sig_off(). You might want to run Sage under gdb with 'sage gdb' to debug this. Sage will now terminate.  Segmentation fault
comment:16 Changed 8 years ago by
comment:17 followup: ↓ 18 Changed 8 years ago by
 Status changed from positive_review to needs_work
Why is this caching desirable? As far as I can see, this creates a memory leak:
i=1 while true: simplicial_complexes.Sphere(i) i=i+1
fails much sooner than it should because all the now useless previous examples are still nailed in memory.
You aren't proposing the cache to save computation time, I suppose? It's probably because you find it desirable to guarantee uniqueness when these things exist (are they parents?) In that case, you can get what you want by inheriting from UniqueRepresentation or UniqueFactory. That keeps a weak cache, so that examples that aren't otherwise referenced anymore can be collected.
comment:18 in reply to: ↑ 17 Changed 8 years ago by
Replying to nbruin:
You aren't proposing the cache to save computation time, I suppose? It's probably because you find it desirable to guarantee uniqueness when these things exist (are they parents?) In that case, you can get what you want by inheriting from UniqueRepresentation or UniqueFactory. That keeps a weak cache, so that examples that aren't otherwise referenced anymore can be collected.
I wonder if we could create a new modifier @weakly_chached_method
(or maybe @cached_method(weakref=True)
) for this purpose. Using a UniqueRepresentation or UniqueFactory probably requires some code rearrangements (for example, Sphere
is a currently a method, not a class whose inheritance we could change).
comment:19 Changed 8 years ago by
Well ... you could write a decorator that turns a function into a UniqueFactory?. However, I think you have a more serious problem: The routines you are now defining have no business being a method (note that you don't use the self
argument!). Python is not Java. You don't have to stuff everything into a class. Much more appropriate is a module (just a file with functions). In fact, since your functions are just interfaces to SimplicialComplex?, any uniqueness of the output value is already ensured by that thing. And if it isn't, then either SimplicialComplex? should be ensuring uniqueness or there's a good reason why it shouldn't and then your routines shouldn't ensure uniqueness either. In other words: the caching you're proposing should either happen on a deeper level or not at all.
For more information about using modules to gather examples or as convenient namespace groupings, see #13115 and this sagedevel thread.
comment:20 followup: ↓ 21 Changed 8 years ago by
If I understand #13115/the thread correctly, a way to do this would be:
 have an
import homology/examples as simplical_complexes
in theall.py
 Make each of these methods into a subclass of
SimplicialComplex
andUniqueRepresentation
and have these be at the module level
since right now SimplicialComplex
is mutable until specified otherwise. (I guess with this we can also do some optimization with certain methods, such as homology for Sphere
... and that should be a different ticket.)
comment:21 in reply to: ↑ 20 ; followup: ↓ 22 Changed 8 years ago by
Replying to tscrim:
If I understand #13115/the thread correctly, a way to do this would be:
 have an
import homology/examples as simplical_complexes
in theall.py
 Make each of these methods into a subclass of
SimplicialComplex
andUniqueRepresentation
and have these be at the module levelsince right now
SimplicialComplex
is mutable until specified otherwise. (I guess with this we can also do some optimization with certain methods, such as homology forSphere
... and that should be a different ticket.)
Yes, that sounds looks like a sensible approach. Additionally, if that seems like a heavyweight solution (it does to me), one should probably see if uniqueness is so important that it warrants a subclass and/or if there are other pressing reasons to make these subclasses. Reasons against uniqueness:
SimplicialComplex
is not a parent so there's no sagetechnical reason to enforce uniqueness (that's just a reason not for uniqueness)
 While any two
Sphere(3)
complexes are isomorphic, they are not canonically so, so mathematically there can be good reasons to have multiple nonidentical spheres in memory.
 It makes things like
Sphere
behave different fromSimplicialComplex
(and more generally not simply be aSimplicialComplex
. That makes code harder to maintain because now you have similarbutsubtlydifferent acting objects. You should only do that if there's a benefit elsewhere.
Should uniqueness of SimplicialComplexes
be desirable in general (why? are they meant to become parents?) then it should perhaps be fixed on that level. Mutable obviously can't be forced to be unique, so then you should probably make two: SimplicialComplex
and MutableSimplicialComplex
(one a subclass of the other if at all possible), where you can make the immutable one unique by also inheriting from UniqueRepresentation?. Going to the immutable one then of course should return an immutable copy, not just change a flag.
Alternatively, (I don't know if that would be too hackish) you could add another flag, canonical_instance=true/false
(only to be used with is_mutable=False
), or make (a renamed version of) is_mutable
tristate, to determines whether a cache is involved. That would have the severe drawback you'd have to break open the caching mechanism used in UniqueRepresentation
.
If you find there's no need for Sphere
etc. to be anything more than just a SimplicialComplex
the implementation becomes really easy: sage.homology.examples.Sphere
can simple be a function, returning the relevant SimplicialComplex
.
comment:22 in reply to: ↑ 21 Changed 8 years ago by
Replying to nbruin:
Reasons against uniqueness:
SimplicialComplex
is not a parent so there's no sagetechnical reason to enforce uniqueness (that's just a reason not for uniqueness)
 While any two
Sphere(3)
complexes are isomorphic, they are not canonically so, so mathematically there can be good reasons to have multiple nonidentical spheres in memory.
That is a good point since I believe not all triangulations of a (high enough dimensional) sphere can be obtained by bistellar flips.
 It makes things like
Sphere
behave different fromSimplicialComplex
(and more generally not simply be aSimplicialComplex
. That makes code harder to maintain because now you have similarbutsubtlydifferent acting objects. You should only do that if there's a benefit elsewhere.
Also a good point.
Should uniqueness of
SimplicialComplexes
be desirable in general (why? are they meant to become parents?) then it should perhaps be fixed on that level. Mutable obviously can't be forced to be unique, so then you should probably make two:SimplicialComplex
andMutableSimplicialComplex
(one a subclass of the other if at all possible), where you can make the immutable one unique by also inheriting from UniqueRepresentation?. Going to the immutable one then of course should return an immutable copy, not just change a flag.
What I was thinking when I created this ticket was that since these are particular examples of simplicial complexes, they should be unique and immutable. However, perhaps it would be better to have them be mutable copies for computations rather than having the user create a copy (I don't really know offhand if these are used computationally in Sage)...I will leave this decision to someone who uses these more than me.
If you find there's no need for
Sphere
etc. to be anything more than just aSimplicialComplex
the implementation becomes really easy:sage.homology.examples.Sphere
can simple be a function, returning the relevantSimplicialComplex
.
If it is decided to essentially do nothing, perhaps we should use this ticket to move everything down to a function.
Also it seems like this needs to be rebased to 5.6.beta1.
Thanks,
Travis
PS  I may not be able to respond for 2 weeks
comment:23 Changed 8 years ago by
Sorry, I did not read the whole discussion, but I see in the patch that cached_method is used. Hence, the constructed simplicial complexes will be kept in memory indefinitely. Wouldn't it be a better idea to turn simplicial_complexes into a UniqueFactory
?
Of course, in a UniqueFactory
one would usually ask for a Klein bottle by simplicial_complexes("KleinBottle")
. But of course one could still define methods KleinBottle
and so on, so that simplicial_complexes.KleinBottle()
is equivalent to simplicial_complexes("KleinBottle")
.
The latter could be automatised by a __getattr__
method:
def __getattr__(self, s): return lambda : self(s)
Why would that solve the problem with permanent caching? Well, UniqueFactory
will soon be using weak dictionaries for caching (in fact it already uses weak references, but improperly, and that's supposed to be fixed in #12215).
comment:24 Changed 8 years ago by
Sorry for falling behind on this ticket. What we should do strongly depends on the answer the question "Should these examples be immutable to begin with (right now one would need to make a mutable copy to manipulate it)?" My belief is still yes and so I'd support going to a UniqueFactory
, however this is at odds with what the graph code does:
sage: g = graphs.StarGraph(5) sage: g Star graph: Graph on 6 vertices sage: g.add_vertex(10) sage: g Star graph: Graph on 7 vertices
Since the graph is given a special name, I'd rather that be immutable. (If we do keep them immutable, we probably should give each of our simplicial complex examples special names too...)
Anyways, what are everyone else's thoughts?
Thanks,
Travis
comment:25 followup: ↓ 26 Changed 8 years ago by
How do you make an immutable simplicial complex mutable? We seem to have missed adding a set_mutable
method in #12587, and so now I don't know how to get a mutable copy of the various examples, short of explicitly setting the _is_mutable
attribute.
I think this should be fixed very soon, maybe before the other issues in this ticket.
As far as whether the examples should be immutable, I don't feel strongly either way. I can see a certain elegance in immutability, but other examples in Sage don't seem to work like this. I suppose we could use UniqueFactory
to create an immutable unique instance, but the functions (or methods, or whatever) could return a mutable copy each time. For example:
sage: MatrixSpace(QQ, 3, 3).identity_matrix().is_mutable() False sage: matrix.identity(QQ, 3).is_mutable() True
This seems rather odd to me, but I suppose it's an option.
comment:26 in reply to: ↑ 25 ; followup: ↓ 27 Changed 8 years ago by
Replying to jhpalmieri:
How do you make an immutable simplicial complex mutable? We seem to have missed adding a
set_mutable
method in #12587, and so now I don't know how to get a mutable copy of the various examples, short of explicitly setting the_is_mutable
attribute.
For example
sage: S = simplicial_complexes.Sphere(4) sage: S.is_mutable() False sage: T = SimplicialComplex(S, is_mutable=True) # This should work and is a bug I didn't catch sage: T.is_mutable() False sage: T = SimplicialComplex(S) # This one I'm happy with leaving immutable sage: T.is_mutable() False
However I do think we should also have a method to make a mutable copy, my suggestions are either copy()
and/or make_mutable()
.
I think this should be fixed very soon, maybe before the other issues in this ticket.
Agreed. This was something I introduced, so I'll be happy to fix it.
As far as whether the examples should be immutable, I don't feel strongly either way. I can see a certain elegance in immutability, but other examples in Sage don't seem to work like this. I suppose we could use
UniqueFactory
to create an immutable unique instance, but the functions (or methods, or whatever) could return a mutable copy each time. For example:sage: MatrixSpace(QQ, 3, 3).identity_matrix().is_mutable() False sage: matrix.identity(QQ, 3).is_mutable() TrueThis seems rather odd to me, but I suppose it's an option.
This is very strange to me as well. Part of the thing is the identity matrix function is cached, so it must return an immutable object.
Best,
Travis
comment:27 in reply to: ↑ 26 Changed 8 years ago by
Replying to tscrim:
However I do think we should also have a method to make a mutable copy, my suggestions are either
copy()
and/ormake_mutable()
.
Another option is to use __copy__()
; then calling copy(X)
would return a mutable copy of X
. We could make copy()
an alias for __copy__()
, so it's visible via tab completion.
comment:28 Changed 8 years ago by
 Dependencies changed from #13244, #12587 to #13244, #12587, #14142
I've started #14142 which will allow mutable copies.
comment:29 Changed 7 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:30 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:31 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:32 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:33 Changed 5 years ago by
 Branch set to u/jhpalmieri/simplicialexamples
comment:34 Changed 5 years ago by
 Commit set to 5bf74b6d7f517c534b668dc602b6c7138729156b
 Description modified (diff)
 Status changed from needs_work to needs_review
Here is a branch which makes two more examples of simplicial complexes immutable, and then converts all of the examples from methods to functions. They are not cached (and I think that was the consensus).
As Travis said above:
If it is decided to essentially do nothing, perhaps we should use this ticket to move everything down to a function.
I guess that's what I've done.
New commits:
fa33dbe  trac 13566: make two more examples of simplicial complexes immutable

5bf74b6  trac 13566: remove the class in sage/homology/examples.py:

A simpler solution would be to turn the constructors in the
SimplicialComplexExamples
class into@cached_methods
. I'm attaching this as a patch.