Opened 5 years ago

Closed 5 years ago

#19065 closed defect (fixed)

A few fixes and enhancements for chain complex morphisms

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-6.9
Component: algebraic topology Keywords:
Cc: tscrim Merged in:
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 94e08a7 (Commits) Commit: 94e08a7d1d7fefdbee992f01604cc48677a70ea2
Dependencies: Stopgaps:

Description

This ticket addresses several issues:

  • when defining chain complex morphisms, we don't check the size of the defining matrices, and we should.
  • the __mul__ method multiplies the factors in the wrong order.
  • it may be useful to have an in_degree method: f.in_degree(5) will return the matrix defining f in degree 5.

Change History (7)

comment:1 Changed 5 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/chains

comment:2 Changed 5 years ago by jhpalmieri

  • Commit set to 45e2206b323ad222e818be931a9e77a4a213b14c
  • Status changed from new to needs_review

New commits:

45e2206trac 19065: fix some issues with chain complex morphisms.

comment:3 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

It looks like you also fixed a bug if there was a mismatch in the matrix dictionaries (which would result it a KeyError being thrown). Do you think you could also add a test for this? If this could never happen, then I'd revert back to directly calling x._matrix_dictionary[i]

Could you also make this doc tweak:

         """
-        The matrix representing this morphism in degree n
+        The matrix representing this morphism in degree `n`.
 
         INPUT:
 
-        - ``n`` - degree
+        - ``n`` -- degree

If you make those changes and the patchbot approves, then you can set this as a positive review on my behalf. Thanks.

comment:4 Changed 5 years ago by jhpalmieri

Good idea. In fact, this led me to find another bug (in the "product" self * x, we need to check that self._domain == x._codomain, not self._codomain == x._domain).

comment:5 Changed 5 years ago by git

  • Commit changed from 45e2206b323ad222e818be931a9e77a4a213b14c to 94e08a7d1d7fefdbee992f01604cc48677a70ea2

Branch pushed to git repo; I updated commit sha1. New commits:

94e08a7trac 19065: one more bug fix plus a doctest

comment:6 Changed 5 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:7 Changed 5 years ago by vbraun

  • Branch changed from u/jhpalmieri/chains to 94e08a7d1d7fefdbee992f01604cc48677a70ea2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.