Opened 10 years ago
Closed 9 years ago
#6099 closed enhancement (fixed)
morphisms of simplicial complexes and the associated chain complex morphisms
Reported by: | bantieau | Owned by: | bantieau |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.3 |
Component: | algebraic topology | Keywords: | |
Cc: | jhpalmieri | Merged in: | sage-4.3.alpha1 |
Authors: | D. Benjamin Antieau | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
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.
Attachments (8)
Change History (25)
Changed 10 years ago by
comment:1 Changed 10 years ago by
Ignore 12335.patch. I would take it off, but I do not know how.
Changed 10 years ago by
comment:2 Changed 10 years ago by
- Summary changed from morphisms of simplicial complexes and the associated chain complex morphisms to [with patch, needs review] morphisms of simplicial complexes and the associated chain complex morphisms
comment:3 Changed 10 years ago by
- Summary changed from [with patch, needs review] morphisms of simplicial complexes and the associated chain complex morphisms to [with patch, needs work] morphisms of simplicial complexes and the associated chain complex morphisms
It's great that you're working on this. It looks very good, but there are a few issues:
- 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
sage: type(S.effective_vertices()) <class 'sage.homology.simplicial_complex.Simplex'>
- I had a few doctest failures which I think are fixed in #6309 (two extra periods), but they should be fixed here.
- Is it a good idea to have
domain
andcodomain
methods for morphisms? I can imagine someone wanting to use the domain of the fiber product, for example, but they won't see the_domain
attribute on tab completion.
- 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
Possibly wrong (function name doesn't occur in doctests): * _repr_(self):
can be avoided if you add "# indirect doctest", like this:sage: x = i.associated_chain_complex_morphism() sage: x # indirect doctest
- Should "product" be renamed "fiber_product" so it's less ambiguous?
- I wonder if
ChainComplexMorphism
should inherit fromModuleElement
rather thanSageObject
. Then you would define a method_mul_
(rather than__mul__
), and similarly for_add_
, and you could also define_lmul_
and_rmul_
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.)
- 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
- D. Benjamin Antieau <d.ben.antieau@gmail.com> (2009.06)
to- \D. Benjamin Antieau <d.ben.antieau@gmail.com> (2009.06)
(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.
Changed 10 years ago by
comment:4 Changed 10 years ago by
- Status changed from needs_work to needs_review
OK. I've uploaded a patch that includes all of the patches above, and also includes all of your recommendations, except for inheriting from ModuleElement?. Perhaps that can be another ticket, if someone wants that. Coverage and doctest are %100.
comment:5 Changed 10 years ago by
- Reviewers set to John Palmieri
- Summary changed from [with patch, needs work] morphisms of simplicial complexes and the associated chain complex morphisms to morphisms of simplicial complexes and the associated chain complex morphisms
This is almost done. I'm attaching a patch making a few changes. First, in homology.rst, it should say sage/homology/chain_complex_homspace
(it has "homset" instead of "homspace"). Also, I think that in category_types.py, the entry for chain complexes should say:
ChainComplexes : [RingModules, AbelianGroups, Sets],\
Also, I've changed the __mul__
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 __rmul__
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.
Finally, the only major problem: my patch fixes an issue with converting maps of simplicial complexes to maps of chain complexes:
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.
The issue is orientation: in the line
mval[X_faces.index(i)+(Y_faces.index(y)*num_faces_X)] = 1
in associated_chain_complex_morphism
, the right side should be 1 or -1, depending on the orientation of y.
I'm giving your patch a positive review. If you're happy with my new patch, change the ticket to "positive review".
comment:6 Changed 10 years ago by
- Status changed from needs_review to positive_review
Release manager: apply only 6099-merged.patch and trac_6099-part2.patch.
comment:7 Changed 10 years ago by
- Merged in set to sage-4.3.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:8 follow-up: ↓ 9 Changed 10 years ago by
- Resolution fixed deleted
- Status changed from closed to new
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.
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 10 years ago by
Replying to mhansen:
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.
Hi Benjamin,
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 ChainComplexes?.ParentMethods? to achieve the current effect.
Good luck, and feel free to bug me.
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 13 Changed 10 years ago by
Replying to nthiery:
Sorry for the conflict. Rebasing the patch should be fairly easy. I suspect that the change to category_types can simply be discarded.
Okay, that's easy enough.
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 ChainComplexes?.ParentMethods? to achieve the current effect.
I'm not sure what ChainComplexes.ParentMethods
means, but we can just define, within the class ChainComplex
, a method _Hom_(self, other)
, right?
comment:11 Changed 10 years ago by
- Status changed from new to needs_review
Here's a rebased version. This makes no changes to category_types.py or to categories/homset.py, instead implementing _Hom_
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.
comment:12 follow-up: ↓ 14 Changed 10 years ago by
I guess the only parts that need review are the two new _Hom_
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...
comment:13 in reply to: ↑ 10 Changed 10 years ago by
Replying to jhpalmieri:
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 ChainComplexes?.ParentMethods? to achieve the current effect.
I'm not sure what
ChainComplexes.ParentMethods
means, but we can just define, within the classChainComplex
, a method_Hom_(self, other)
, right?
Indeed.
ChainComplexes.ParentMethods
is the class in the ChainComplexes
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.
comment:14 in reply to: ↑ 12 Changed 10 years ago by
- Status changed from needs_review to positive_review
Replying to jhpalmieri:
I guess the only parts that need review are the two new
_Hom_
methods, one in chain_complex.py and one in simplicial_complex.py.
I just looked at those, and this sounds good. I am setting the positive review back. Thanks!
And then there is the issue of whether it plays well with the new category stuff...
At first sight, it should work smoothly.
For the record: in the new category code, when a category is passed as optional argument, it's done as category=...
rather than cat=...
. I just checked, and this should not be an issue for _Hom_. You may want to change this for consistency though, now or later.
comment:15 follow-up: ↓ 16 Changed 10 years ago by
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 _Hom_
methods.)
comment:16 in reply to: ↑ 15 Changed 10 years ago by
Replying to jhpalmieri:
I'll put up a new patch in just a minute.
Thanks!
(I was just imitating the code in rings/number_field/number_field.py and structure/parent.pyx, two places where I found pre-existing
_Hom_
methods.)
Yup, you had no chance to guess otherwise. This part is seriously missing documentation.
comment:17 Changed 9 years ago by
- Merged in changed from sage-4.3.alpha0 to sage-4.3.alpha1
- Report Upstream set to N/A
- Resolution set to fixed
- Status changed from positive_review to closed
This patch implements the chain complex morphisms part of this trac ticket.