Sage: Ticket #6099: morphisms of simplicial complexes and the associated chain complex morphisms
https://trac.sagemath.org/ticket/6099
<p>
Add functionality to sage to create morphisms between simplicial complexes, and to associate to these morphisms the chain complex maps on the homological and cohomological chain complexes.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/6099
Trac 1.1.6bantieauMon, 01 Jun 2009 23:10:24 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>12335.patch</em>
</li>
</ul>
<p>
This patch implements the chain complex morphisms part of this trac ticket.
</p>
TicketbantieauMon, 01 Jun 2009 23:14:34 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>6099-1.patch</em>
</li>
</ul>
<p>
This patch implements the chain complex morphisms part of this trac ticket.
</p>
TicketbantieauMon, 01 Jun 2009 23:15:19 GMT
https://trac.sagemath.org/ticket/6099#comment:1
https://trac.sagemath.org/ticket/6099#comment:1
<p>
Ignore 12335.patch. I would take it off, but I do not know how.
</p>
TicketbantieauTue, 16 Jun 2009 06:17:28 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>6099-2.patch</em>
</li>
</ul>
TicketbantieauTue, 16 Jun 2009 06:21:12 GMTsummary changed
https://trac.sagemath.org/ticket/6099#comment:2
https://trac.sagemath.org/ticket/6099#comment:2
<ul>
<li><strong>summary</strong>
changed from <em>morphisms of simplicial complexes and the associated chain complex morphisms</em> to <em>[with patch, needs review] morphisms of simplicial complexes and the associated chain complex morphisms</em>
</li>
</ul>
TicketbantieauWed, 17 Jun 2009 00:50:46 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>6099-3.patch</em>
</li>
</ul>
<p>
tweak to be compatibe with <a class="closed ticket" href="https://trac.sagemath.org/ticket/6141" title="enhancement: [with patch, positive review] simplicial complexes: change 'facets' ... (closed: fixed)">#6141</a>, which changes facets to facets().
</p>
TicketjhpalmieriWed, 17 Jun 2009 18:19:24 GMTsummary changed
https://trac.sagemath.org/ticket/6099#comment:3
https://trac.sagemath.org/ticket/6099#comment:3
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] morphisms of simplicial complexes and the associated chain complex morphisms</em> to <em>[with patch, needs work] morphisms of simplicial complexes and the associated chain complex morphisms</em>
</li>
</ul>
<p>
It's great that you're working on this. It looks very good, but there are a few issues:
</p>
<ul><li>in the docstring for effective_vertices, you might mention that it returns a Simplex. The docstring could start "The set of vertices belonging to some face, as a simplex", and you could also put in a doctest like
<pre class="wiki">sage: type(S.effective_vertices())
<class 'sage.homology.simplicial_complex.Simplex'>
</pre></li></ul><ul><li>I had a few doctest failures which I think are fixed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/6309" title="enhancement: miscellaneous additions to simplicial complex class; clique complex ... (closed: fixed)">#6309</a> (two extra periods), but they should be fixed here.
</li></ul><ul><li>Is it a good idea to have <code>domain</code> and <code>codomain</code> methods for morphisms? I can imagine someone wanting to use the domain of the fiber product, for example, but they won't see the <code>_domain</code> attribute on tab completion.
</li></ul><ul><li>You don't have 100% documentation and doctest coverage. Type 'sage -coverage ...insert_path_here.../sage/homology' to get a summary. When you do this, note that messages like
<pre class="wiki"> Possibly wrong (function name doesn't occur in doctests):
* _repr_(self):
</pre>can be avoided if you add "# indirect doctest", like this:
<pre class="wiki"> sage: x = i.associated_chain_complex_morphism()
sage: x # indirect doctest
</pre></li></ul><ul><li>Should "product" be renamed "fiber_product" so it's less ambiguous?
</li></ul><ul><li>I wonder if <code>ChainComplexMorphism</code> should inherit from <code>ModuleElement</code> rather than <code>SageObject</code>. Then you would define a method <code>_mul_</code> (rather than <code>__mul__</code>), and similarly for <code>_add_</code>, and you could also define <code>_lmul_</code> and <code>_rmul_</code> to deal with scalar multiplication. Check the Sage reference manual, the "Coercion" section. (This section is newly added to the reference manual, as of Sage 4.0.1 or 4.0.2, I think.)
</li></ul><ul><li>I'm attaching a small patch which adds the new files to the reference manual. (This involves editing one file, doc/en/reference/homology.rst, and also because of the way reST works, I had to change
<pre class="wiki">- D. Benjamin Antieau <d.ben.antieau@gmail.com> (2009.06)
</pre>to
<pre class="wiki">- \D. Benjamin Antieau <d.ben.antieau@gmail.com> (2009.06)
</pre>(The "D." at essentially the beginning of the line seemed to tell Sphinx that this was part of a numbered list, starting with number 4.) Feel free to incorporate my changes and produce one single patch which does everything here.
</li></ul>
TicketjhpalmieriWed, 17 Jun 2009 18:19:40 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>ref_6099.patch</em>
</li>
</ul>
TicketbantieauFri, 06 Nov 2009 21:18:57 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>6099-merged.patch</em>
</li>
</ul>
<p>
Hopefully final merged patch.
</p>
TicketbantieauFri, 06 Nov 2009 21:20:28 GMTstatus changed
https://trac.sagemath.org/ticket/6099#comment:4
https://trac.sagemath.org/ticket/6099#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
OK. I've uploaded a patch that includes all of the patches above, and also includes all of your recommendations, except for inheriting from <a class="missing wiki">ModuleElement?</a>. Perhaps that can be another ticket, if someone wants that. Coverage and doctest are %100.
</p>
TicketjhpalmieriTue, 10 Nov 2009 22:59:13 GMTsummary changed; reviewer, author set
https://trac.sagemath.org/ticket/6099#comment:5
https://trac.sagemath.org/ticket/6099#comment:5
<ul>
<li><strong>reviewer</strong>
set to <em>John Palmieri</em>
</li>
<li><strong>author</strong>
set to <em>D. Benjamin Antieau</em>
</li>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] morphisms of simplicial complexes and the associated chain complex morphisms</em> to <em>morphisms of simplicial complexes and the associated chain complex morphisms</em>
</li>
</ul>
<p>
This is almost done. I'm attaching a patch making a few changes. First, in homology.rst, it should say <code>sage/homology/chain_complex_homspace</code> (it has "homset" instead of "homspace"). Also, I think that in category_types.py, the entry for chain complexes should say:
</p>
<pre class="wiki"> ChainComplexes : [RingModules, AbelianGroups, Sets],\
</pre><p>
Also, I've changed the <code>__mul__</code> method for maps of chain complexes so that when the right-hand factor is a ring element, it gets multiplied on the right, not the left (in case we ever work over noncommutative rings). I've also added an <code>__rmul__</code> method for multiplying on the left by a ring element. I changed the string representation for chain complexes so it doesn't have a period at the end, so that your string representations for chain maps look better.
</p>
<p>
Finally, the only major problem: my patch fixes an issue with converting maps of simplicial complexes to maps of chain complexes:
</p>
<pre class="wiki">sage: X = SimplicialComplex(1, [[0,1]]); X
Simplicial complex with vertex set (0, 1) and facets {(0, 1)}
sage: H = Hom(X, X)
sage: f = H({0:1, 1:0})
sage: f.associated_chain_complex_morphism()
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
/Users/palmieri/.sage/temp/jpalmieri538.local/84693/_Users_palmieri__sage_init_sage_0.py in <module>()
/Applications/sage/local/lib/python2.6/site-packages/sage/homology/simplicial_complex_morphism.pyc in associated_chain_complex_morphism(self, base_ring, augmented, cochain)
322 return ChainComplexMorphism(matrices,\
323 self._domain.chain_complex(base_ring=base_ring,augmented=augmented,cochain=cochain),\
--> 324 self._codomain.chain_complex(base_ring=base_ring,augmented=augmented,cochain=cochain))
325 else:
326 return ChainComplexMorphism(matrices,\
/Applications/sage/local/lib/python2.6/site-packages/sage/homology/chain_complex_morphism.pyc in __init__(self, matrices, C, D)
132 if (i+1) in C.differential().keys() and (i+1) in D.differential().keys():
133 if not matrices[i]*C.differential()[i+1]==D.differential()[i+1]*matrices[i+1]:
--> 134 raise ValueError, "Matrices must define a chain complex morphism."
135 elif (i+1) in C.differential().keys():
136 if not matrices[i]*C.differential()[i+1].is_zero():
ValueError: Matrices must define a chain complex morphism.
</pre><p>
The issue is orientation: in the line
</p>
<pre class="wiki"> mval[X_faces.index(i)+(Y_faces.index(y)*num_faces_X)] = 1
</pre><p>
in <code>associated_chain_complex_morphism</code>, the right side should be 1 or -1, depending on the orientation of y.
</p>
<p>
I'm giving your patch a positive review. If you're happy with my new patch, change the ticket to "positive review".
</p>
TicketjhpalmieriTue, 10 Nov 2009 23:38:12 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>trac_6099-part2.patch</em>
</li>
</ul>
<p>
apply on top of 6099-merged.patch
</p>
TicketbantieauSat, 14 Nov 2009 22:49:42 GMTstatus changed
https://trac.sagemath.org/ticket/6099#comment:6
https://trac.sagemath.org/ticket/6099#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Release manager: apply only 6099-merged.patch and trac_6099-part2.patch.
</p>
TicketmhansenTue, 17 Nov 2009 06:18:08 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/6099#comment:7
https://trac.sagemath.org/ticket/6099#comment:7
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.3.alpha0</em>
</li>
</ul>
TicketmhansenTue, 17 Nov 2009 14:56:30 GMTstatus changed; resolution deleted
https://trac.sagemath.org/ticket/6099#comment:8
https://trac.sagemath.org/ticket/6099#comment:8
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
</ul>
<p>
I had to back this out for now due to conflicts with the category code. I'll look at readding this once those patches are merged.
</p>
TicketnthieryTue, 17 Nov 2009 15:10:13 GMT
https://trac.sagemath.org/ticket/6099#comment:9
https://trac.sagemath.org/ticket/6099#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/6099#comment:8" title="Comment 8">mhansen</a>:
</p>
<blockquote class="citation">
<p>
I had to back this out for now due to conflicts with the category code. I'll look at readding this once those patches are merged.
</p>
</blockquote>
<blockquote>
<p>
Hi Benjamin,
</p>
</blockquote>
<p>
Sorry for the conflict. Rebasing the patch should be fairly easy. I suspect that the change to category_types can simply be discarded. As for the change in homset.py: I have removed this ugly run time type checking there. Instead, Hom(X, Y) looks for a method X._Hom_, and calls it if it exists. This _Hom_ method could typically be implemented in <a class="missing wiki">ChainComplexes?</a>.<a class="missing wiki">ParentMethods?</a> to achieve the current effect.
</p>
<p>
Good luck, and feel free to bug me.
</p>
TicketjhpalmieriTue, 17 Nov 2009 18:09:57 GMT
https://trac.sagemath.org/ticket/6099#comment:10
https://trac.sagemath.org/ticket/6099#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/6099#comment:9" title="Comment 9">nthiery</a>:
</p>
<blockquote class="citation">
<p>
Sorry for the conflict. Rebasing the patch should be fairly easy. I suspect that the change to category_types can simply be discarded.
</p>
</blockquote>
<p>
Okay, that's easy enough.
</p>
<blockquote class="citation">
<p>
As for the change in homset.py: I have removed this ugly run time type checking there. Instead, Hom(X, Y) looks for a method X._Hom_, and calls it if it exists. This _Hom_ method could typically be implemented in <a class="missing wiki">ChainComplexes?</a>.<a class="missing wiki">ParentMethods?</a> to achieve the current effect.
</p>
</blockquote>
<p>
I'm not sure what <code>ChainComplexes.ParentMethods</code> means, but we can just define, within the class <code>ChainComplex</code>, a method <code>_Hom_(self, other)</code>, right?
</p>
TicketjhpalmieriTue, 17 Nov 2009 20:24:42 GMTstatus changed
https://trac.sagemath.org/ticket/6099#comment:11
https://trac.sagemath.org/ticket/6099#comment:11
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
Here's a rebased version. This makes no changes to category_types.py or to categories/homset.py, instead implementing <code>_Hom_</code> methods for simplicial complexes and chain complexes. With Sage 4.2.1, it applies cleanly and passes all tests. If it still causes problems when merged, we'll probably have to wait until 4.3.alpha0 is released and work from there.
</p>
TicketjhpalmieriTue, 17 Nov 2009 20:25:53 GMT
https://trac.sagemath.org/ticket/6099#comment:12
https://trac.sagemath.org/ticket/6099#comment:12
<p>
I guess the only parts that need review are the two new <code>_Hom_</code> methods, one in chain_complex.py and one in simplicial_complex.py. And then there is the issue of whether it plays well with the new category stuff...
</p>
TicketnthieryTue, 17 Nov 2009 20:34:35 GMT
https://trac.sagemath.org/ticket/6099#comment:13
https://trac.sagemath.org/ticket/6099#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/6099#comment:10" title="Comment 10">jhpalmieri</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
As for the change in homset.py: I have removed this ugly run time type checking there. Instead, Hom(X, Y) looks for a method X._Hom_, and calls it if it exists. This _Hom_ method could typically be implemented in <a class="missing wiki">ChainComplexes?</a>.<a class="missing wiki">ParentMethods?</a> to achieve the current effect.
</p>
</blockquote>
<p>
I'm not sure what <code>ChainComplexes.ParentMethods</code> means, but we can just define, within the class <code>ChainComplex</code>, a method <code>_Hom_(self, other)</code>, right?
</p>
</blockquote>
<p>
Indeed.
</p>
<p>
<code>ChainComplexes.ParentMethods</code> is the class in the <code>ChainComplexes</code> category containing the generic code that applies to all parents in this category. That could be useful later on if there is more than one implementation of such parents. No rush for now.
</p>
TicketnthieryTue, 17 Nov 2009 20:40:29 GMTstatus changed
https://trac.sagemath.org/ticket/6099#comment:14
https://trac.sagemath.org/ticket/6099#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/6099#comment:12" title="Comment 12">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
I guess the only parts that need review are the two new <code>_Hom_</code> methods, one in chain_complex.py and one in simplicial_complex.py.
</p>
</blockquote>
<p>
I just looked at those, and this sounds good. I am setting the positive review back. Thanks!
</p>
<blockquote class="citation">
<p>
And then there is the issue of whether it plays well with the new category stuff...
</p>
</blockquote>
<p>
At first sight, it should work smoothly.
</p>
<p>
For the record: in the new category code, when a category is passed as optional argument, it's done as <code>category=...</code> rather than <code>cat=...</code>. I just checked, and this should not be an issue for _Hom_. You may want to change this for consistency though, now or later.
</p>
TicketjhpalmieriTue, 17 Nov 2009 20:44:06 GMT
https://trac.sagemath.org/ticket/6099#comment:15
https://trac.sagemath.org/ticket/6099#comment:15
<p>
I'll put up a new patch in just a minute. (I was just imitating the code in rings/number_field/number_field.py and structure/parent.pyx, two places where I found pre-existing <code>_Hom_</code> methods.)
</p>
TicketnthieryTue, 17 Nov 2009 23:28:49 GMT
https://trac.sagemath.org/ticket/6099#comment:16
https://trac.sagemath.org/ticket/6099#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/6099#comment:15" title="Comment 15">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
I'll put up a new patch in just a minute.
</p>
</blockquote>
<p>
Thanks!
</p>
<blockquote class="citation">
<p>
(I was just imitating the code in rings/number_field/number_field.py and structure/parent.pyx, two places where I found pre-existing <code>_Hom_</code> methods.)
</p>
</blockquote>
<p>
Yup, you had no chance to guess otherwise. This part is seriously missing documentation.
</p>
TicketjhpalmieriTue, 24 Nov 2009 19:46:51 GMTattachment set
https://trac.sagemath.org/ticket/6099
https://trac.sagemath.org/ticket/6099
<ul>
<li><strong>attachment</strong>
set to <em>trac_6099-rebased.patch</em>
</li>
</ul>
<p>
rebased version of patch. apply only this file.
</p>
TicketmhansenSun, 29 Nov 2009 04:55:49 GMTstatus, merged changed; resolution, upstream set
https://trac.sagemath.org/ticket/6099#comment:17
https://trac.sagemath.org/ticket/6099#comment:17
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
changed from <em>sage-4.3.alpha0</em> to <em>sage-4.3.alpha1</em>
</li>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
</ul>
Ticket