Opened 8 years ago

Closed 8 years ago

#10974 closed defect (fixed)

Overhaul matrix stack, augment

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-4.7
Component: linear algebra Keywords:
Cc: Merged in: sage-4.7.alpha4
Authors: Rob Beezer Reviewers: Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This patch overhauls, consolidates, fixes and expands matrix augmentation and stacking.

Fixes:

  • subdivisions of matrices are generally not preserved under these operations, but now are

Features:

  • a vector may now be supplied as a second argument across all routines

Consolidation:

  • there is some standardization of variable names and techniques across the routines

Expansion:

  • augment() for dense integer matrices is uncommented and working now

Documentation:

  • new features (vector argument, subdivisions) have been documented where added
  • a general upgrade in documentation would be valuable, on another ticket

To Do:

  • work to have result formed over a common ring

"general" patch adds vector argument and subdivision support to the generic stack() method. Subdivision management is split out as helper methods for use with routines in the "special" patch.

"special" patch works on routines for dense integer matrices, mod 2 matrices, and generic sparse matrices. It also moves the generic matrix augment() to a better location in the source (it was left in place so changes could be seen more readily on the "general" patch).

Apply:

  1. trac_10974_stack-augment-general.patch
  2. trac_10974_stack-augment-special.patch
  3. trac_10974_stack-augment.reviewer-v2.patch

Attachments (4)

trac_10974_stack-augment-general.patch (17.2 KB) - added by rbeezer 8 years ago.
trac_10974_stack-augment-special.patch (33.0 KB) - added by rbeezer 8 years ago.
trac_10974_stack-augment.reviewer.patch (7.1 KB) - added by kini 8 years ago.
line wrapping
trac_10974_stack-augment.reviewer-v2.patch (7.1 KB) - added by rbeezer 8 years ago.

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by rbeezer

Changed 8 years ago by rbeezer

comment:1 Changed 8 years ago by rbeezer

  • Authors set to Rob Beezer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by kini

  • Reviewers set to Keshav Kini
  • Status changed from needs_review to positive_review

Three doctests failed on sage.math. I reran them and they failed to fail (one of them was devel/sage/sage/tests/startup.py ), so everything looks good. I did some minor formatting to keep line lengths down according to PEP 8 and will attach a reviewer patch accordingly, but feel free to veto it. Positive review!

Changed 8 years ago by kini

line wrapping

comment:3 Changed 8 years ago by kini

  • Description modified (diff)

(for patchbot)

comment:4 Changed 8 years ago by rbeezer

Keshav,

Thanks for the review! The changes look good - thanks for those as well.

Rob

comment:5 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Change the commit message of the last patch such that it contains the ticket number.

Changed 8 years ago by rbeezer

comment:6 Changed 8 years ago by rbeezer

  • Description modified (diff)
  • Status changed from needs_work to positive_review

Commit message fixed in v2 version of reviewer patch.

comment:7 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This patch conflicts with #4983.

comment:9 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to positive_review

Let's leave this one alone. I'll rebase #4983. I'm setting this back to "positive review".

comment:10 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.