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)

12335.patch (28.9 KB) - added by bantieau 10 years ago.
This patch implements the chain complex morphisms part of this trac ticket.
6099-1.patch (28.9 KB) - added by bantieau 10 years ago.
This patch implements the chain complex morphisms part of this trac ticket.
6099-2.patch (29.7 KB) - added by bantieau 10 years ago.
6099-3.patch (3.3 KB) - added by bantieau 10 years ago.
tweak to be compatibe with #6141, which changes facets to facets().
ref_6099.patch (2.4 KB) - added by jhpalmieri 10 years ago.
6099-merged.patch (52.7 KB) - added by bantieau 10 years ago.
Hopefully final merged patch.
trac_6099-part2.patch (19.3 KB) - added by jhpalmieri 10 years ago.
apply on top of 6099-merged.patch
trac_6099-rebased.patch (58.8 KB) - added by jhpalmieri 10 years ago.
rebased version of patch. apply only this file.

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by bantieau

This patch implements the chain complex morphisms part of this trac ticket.

Changed 10 years ago by bantieau

This patch implements the chain complex morphisms part of this trac ticket.

comment:1 Changed 10 years ago by bantieau

Ignore 12335.patch. I would take it off, but I do not know how.

Changed 10 years ago by bantieau

comment:2 Changed 10 years ago by bantieau

  • 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

Changed 10 years ago by bantieau

tweak to be compatibe with #6141, which changes facets to facets().

comment:3 Changed 10 years ago by jhpalmieri

  • 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 and codomain 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 from ModuleElement rather than SageObject. 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 jhpalmieri

Changed 10 years ago by bantieau

Hopefully final merged patch.

comment:4 Changed 10 years ago by bantieau

  • 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 jhpalmieri

  • Authors set to D. Benjamin Antieau
  • 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".

Changed 10 years ago by jhpalmieri

apply on top of 6099-merged.patch

comment:6 Changed 10 years ago by bantieau

  • 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 mhansen

  • Merged in set to sage-4.3.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:8 follow-up: Changed 10 years ago by mhansen

  • 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: Changed 10 years ago by nthiery

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: Changed 10 years ago by jhpalmieri

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 jhpalmieri

  • 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: Changed 10 years ago by 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. 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 nthiery

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 class ChainComplex, 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 nthiery

  • 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: Changed 10 years ago by jhpalmieri

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 nthiery

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.

Changed 10 years ago by jhpalmieri

rebased version of patch. apply only this file.

comment:17 Changed 9 years ago by mhansen

  • 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
Note: See TracTickets for help on using tickets.