Opened 11 years ago

Closed 11 years ago

#10424 closed enhancement (fixed)

Expand matrix augment'ing to allow vectors as input

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-4.6.2
Component: linear algebra Keywords:
Cc: jason, robertwb, jvkersch Merged in: sage-4.6.2.alpha0
Authors: Rob Beezer Reviewers: Joris Vankerschaver
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

A common situation for augmenting a matrix is when a beginning student might augment a coefficient matrix with a single (column) vector, and then analyze the reduced row echelon form to uncover the nature of the solution set. Right now the vector needs to be converted by hand into a matrix with a single column (not trivial for a beginner).

This patch makes this possible easily. In detail, it:

  1. Demotes the current augment() method to a helper method (so that its optimized functionality is unchanged).
  1. Makes a new augment() method. Its main purpose is to allow for either a matrix or a vector as input, and then error-check and/or sanitize the input for the helper function.
  1. Documentation has improved, as written up on the new method, which is what a user will see.

This work suggests two further possible improvements.

  1. Add an option to subdivide the augmented matrix. An upcoming patch will do this (and for stack() as well).
  1. Coerce to a common parent, rather than coercing the elements of the other matrix into the base ring of self.

Attachments (1)

trac_10424-matrix-augment-vector.patch (9.6 KB) - added by rbeezer 11 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by rbeezer

  • Cc jason added
  • Status changed from new to needs_review

comment:2 Changed 11 years ago by robertwb

  • Cc robertwb added

comment:3 follow-up: Changed 11 years ago by robertwb

The current augment doesn't really have any optimized functionality, so I think it'd make more sense to put this all into a single function. Just do

cdef Matrix other

and name the original input to be something else.

comment:4 in reply to: ↑ 3 Changed 11 years ago by rbeezer

  • Status changed from needs_review to needs_work

Replying to robertwb:

The current augment doesn't really have any optimized functionality, so I think it'd make more sense to put this all into a single function. Just do

cdef Matrix other

and name the original input to be something else.

Yes, I was wrestling with the cdef for other and didn't want to slow down any of the optimized stuff there. It would be better to make this all one function. So I'll take your hint and try to make it happen.

I did notice that matrix row operations are implemented in two parts (a pure python routine and a Cython routine), but maybe that is because the Cython part gets used twice typically.

Thanks for the assist and I'll get back to this soon.

Rob

comment:5 Changed 11 years ago by rbeezer

  • Status changed from needs_work to needs_review

Replacement patch consolidates everything back into the single augment() method. I have also pulled in the subdivision code from #10425, which will now make that ticket obsolete. But go look at #10425 for a brief explanation of decisions about subdivisions. I'll visit stack() once this is done.

Robert - thanks for the help, that was enough to get me on-track.

New patch passes all the doctests from before with no changes, and all of the sage/matrix directory. HTML documentation checks out.

comment:6 follow-up: Changed 11 years ago by jvkersch

I played with the patch, everything works as expected, HTML doesn't generate any warnings (and the docstrings are very clear and to the point -- a pleasure to read). I'm currently running make ptestlong, and I can give the patch a positive review once that finishes.

I agree that it would be more natural to coerce the elements of self and other into some common base ring -- this is the behavior that I intuitively expected, but I didn't see the "future improvements" description on this patch.

comment:7 Changed 11 years ago by jvkersch

  • Cc jvkersch added
  • Owner changed from jason, was to jvkersch

comment:8 follow-up: Changed 11 years ago by jvkersch

  • Owner changed from jvkersch to jason, was

Oops -- sorry!

comment:9 in reply to: ↑ 8 ; follow-up: Changed 11 years ago by rbeezer

Replying to jvkersch:

Oops -- sorry!

I think this is a bug in trac, I also double-check that "assign to" hasn't flipped automatically to me. ;-)

comment:10 in reply to: ↑ 6 Changed 11 years ago by rbeezer

Replying to jvkersch:

I agree that it would be more natural to coerce the elements of self and other into some common base ring -- this is the behavior that I intuitively expected, but I didn't see the "future improvements" description on this patch.

Thanks for the look! I could open a new ticket for common parents, maybe after I go mess with stack() so that it will behave similarly. When I thought about this I couldn't find the common parent function - I know it is out there somewhere.

Rob

comment:11 in reply to: ↑ 9 ; follow-up: Changed 11 years ago by jason

Replying to rbeezer:

Replying to jvkersch: I also double-check that "assign to" hasn't flipped automatically to me. ;-)

Bug? That's a feature! It makes the person complaining about something responsible for the work :).

comment:12 in reply to: ↑ 11 ; follow-up: Changed 11 years ago by jvkersch

  • Reviewers set to jvkersch
  • Status changed from needs_review to positive_review

Doctests pass on OS X 10.6, so I'm giving this a positive review.

I'll keep an eye open for any tickets that follow up on this, please CC me if you open any. Thanks also for pointing out the "assign to" issue.

I also double-check that "assign to" hasn't flipped automatically to me. ;-)

Bug? That's a feature! It makes the person complaining about something responsible for the work :).

For some reason, this reminds me of the way my wife assigns me household chores...

comment:13 Changed 11 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-4.6.2
  • Reviewers changed from jvkersch to Joris Vankerschaver

comment:14 in reply to: ↑ 12 Changed 11 years ago by rbeezer

Replying to jvkersch:

Thanks again for the review, Joris.

I'll keep an eye open for any tickets that follow up on this, please CC me if you open any. Thanks also for pointing out the "assign to" issue.

I'll likely extend stack() to match when I get a chance, and will cc you. Thanks for the interest.

For some reason, this reminds me of the way my wife assigns me household chores...

Hmmm. ;-)

Rob

comment:15 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Please fix the ticket number in the commit message.

comment:16 follow-up: Changed 11 years ago by jason

Does the release manager's script automatically add the correct ticket number to commit messages if the commit message doesn't start with the ticket number? I thought someone added that feature.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 11 years ago by jdemeyer

Replying to jason:

Does the release manager's script automatically add the correct ticket number to commit messages if the commit message doesn't start with the ticket number?

The release manager script detects that situation, but does not fix it. I guess it would not be easy to do that, since you need to implement logging in to Trac, adding a patch, maybe adding a comment.

Changed 11 years ago by rbeezer

comment:18 Changed 11 years ago by rbeezer

  • Status changed from needs_work to positive_review

OK, replaced the patch with one using the correct number in the commit message. Sorry for the bother.

comment:19 in reply to: ↑ 17 ; follow-up: Changed 11 years ago by jason

Replying to jdemeyer:

Replying to jason:

Does the release manager's script automatically add the correct ticket number to commit messages if the commit message doesn't start with the ticket number?

The release manager script detects that situation, but does not fix it. I guess it would not be easy to do that, since you need to implement logging in to Trac, adding a patch, maybe adding a comment.

Ah. I was thinking that the release manager's script would just edit the commit message in the patch by automatically, but not bother to post a new patch to trac.

I would *love* if the release manager's script would just automatically put the ticket number on, as a standard part of importing a patch. If this was accepted practice, it would cut down on yet one more administrative annoyance which has room for human error in posting a patch. Not to mention that it would then make the format consistent, which means tools could more easily search/filter/do stuff with the hg commit messages.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 11 years ago by jdemeyer

Replying to jason:

I would *love* if the release manager's script would just automatically put the ticket number on, as a standard part of importing a patch. If this was accepted practice, it would cut down on yet one more administrative annoyance which has room for human error in posting a patch. Not to mention that it would then make the format consistent, which means tools could more easily search/filter/do stuff with the hg commit messages.

That would be easy to implement, but then we would need people to suddenly not put ticket numbers themselves in the commit message. The best solution would be to add another "header" field, so the top of a patch file would look like

# HG changeset patch
# User Rob Beezer <beezer@ups.edu>
# Date 1291401373 28800
# Node ID f1dfd7d6d7fcea5f8473979c4052824b2a8579f1
# Parent  f843fff860efc5c791da4d8f7d85d434ba057de9
# Ticket 10242
10424: augmentation of matrix augment method

But can hg cope with this and can we grep for it?

comment:21 in reply to: ↑ 20 Changed 11 years ago by robertwb

Replying to jdemeyer:

Replying to jason:

I would *love* if the release manager's script would just automatically put the ticket number on, as a standard part of importing a patch. If this was accepted practice, it would cut down on yet one more administrative annoyance which has room for human error in posting a patch. Not to mention that it would then make the format consistent, which means tools could more easily search/filter/do stuff with the hg commit messages.

That would be easy to implement, but then we would need people to suddenly not put ticket numbers themselves in the commit message.

Why? Wouldn't it be easier to have a script that prepends the ticket number if it isn't already in the description?

The best solution would be to add another "header" field, so the top of a patch file would look like

# HG changeset patch
# User Rob Beezer <beezer@ups.edu>
# Date 1291401373 28800
# Node ID f1dfd7d6d7fcea5f8473979c4052824b2a8579f1
# Parent  f843fff860efc5c791da4d8f7d85d434ba057de9
# Ticket 10242
10424: augmentation of matrix augment method

But can hg cope with this and can we grep for it?

I don't know, but it wouldn't show up in the shortlogs, so better to have it in the first line of the description.

In any case, there should be IMHO a different setting than "needs work" for trivial issues like this.

comment:22 Changed 11 years ago by jdemeyer

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