Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10944 closed enhancement (fixed)

Check for similar matrices

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

Description (last modified by rbeezer)

Method checks if two matrices are similar. This is limited by the need to eventually compute a canonical form, in this case the Jordan form.

It will always compute a result for matrices over the rationals, owing to QQbar and can often arrive at a result of False without going as far as a Jordan form.

Apply:

  1. trac_10944-similar-matrices-v2.patch
  2. trac_10944-similar-matrices-minor-fixes.patch

Attachments (3)

trac_10944-similar-matrices.patch (12.2 KB) - added by rbeezer 8 years ago.
trac_10944-similar-matrices-v2.patch (12.1 KB) - added by ddrake 8 years ago.
rebased for 4.7.1.alpha1; apply only this
trac_10944-similar-matrices-minor-fixes.patch (1.0 KB) - added by rbeezer 8 years ago.

Download all attachments as: .zip

Change History (21)

Changed 8 years ago by rbeezer

comment:1 Changed 8 years ago by rbeezer

  • Authors set to Rob Beezer
  • Status changed from new to needs_review

comment:2 follow-up: Changed 8 years ago by ddrake

  • Reviewers set to Dan Drake
  • Status changed from needs_review to positive_review

This is very nice work. Applies and passes doctests in 4.6.2 and 4.7.rc0. Positive review.

comment:3 in reply to: ↑ 2 Changed 8 years ago by rbeezer

Replying to ddrake:

Thanks for your help with all this linear algebra code.

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

comment:5 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Needs to be rebased to sage-4.7.1.alpha1:

patching file sage/matrix/matrix2.pyx
Hunk #1 FAILED at 6299.
1 out of 1 hunk FAILED -- saving rejects to file sage/matrix/matrix2.pyx.rej

Changed 8 years ago by ddrake

rebased for 4.7.1.alpha1; apply only this

comment:6 follow-up: Changed 8 years ago by ddrake

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

Hah. The code that caused the conflict was another one of Rob's patches. :)

Rebased for 4.7.1.alpha1.

comment:7 in reply to: ↑ 6 Changed 8 years ago by rbeezer

Replying to ddrake:

Hah. The code that caused the conflict was another one of Rob's patches. :)

Rebased for 4.7.1.alpha1.

Hi Dan,

I'm a couple more time zones closer to you at the moment. ;-)

Thanks for the rebase, this could have fallen through the cracks for a while.

As of today, I have rational canonical form (aka Frobenius form) working, so I should be able to eventually upgrade this to provide a yes/no answer over any field.

Rob

comment:8 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 follow-up: Changed 8 years ago by kini

Someone on IRC (Alligadi) voiced concerns about lines 7417 and 7422 of sage/matrix/matrix2.pyx as added in the patch, namely that there are call parens missing from 7417 and that the second self should be other in 7422. Typos?

comment:10 in reply to: ↑ 9 Changed 8 years ago by rbeezer

Replying to kini:

Someone on IRC (Alligadi) voiced concerns about lines 7417 and 7422 of sage/matrix/matrix2.pyx as added in the patch, namely that there are call parens missing from 7417 and that the second self should be other in 7422. Typos?

Correct. Both are small mistakes. No harm done, really, since the only consequence is the failure to avoid unnecessary computation. In other words, results will be correct as written, but intended efficiencies will not happen. I'll put up a patch tomorrow or so.

Thanks for passing along the IRC intel.

Rob

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

Should I reopen the ticket so you can make the changes here?

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

Replying to jdemeyer:

Should I reopen the ticket so you can make the changes here?

Probably best to just reopen this, I should have a patch up right away.

Rob

comment:13 Changed 8 years ago by rbeezer

  • Description modified (diff)

Observed mistakes fixed in small patch just added. I built this on 4.7.1.alpha2, but it should apply on any version that the main patch is applied.

No point in adding anything to the doctests, since these errors only lengthen execution time and no reasonable doctest could tell.

Jeroen - this can move to "needs review" once you reopen it.

Kini and Alligadi - thanks for the report!

Rob

comment:14 Changed 8 years ago by jdemeyer

  • Merged in sage-4.7.1.alpha2 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:15 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:16 follow-up: Changed 8 years ago by ddrake

  • Status changed from needs_review to positive_review

Oops, I should have caught those two little errors in my first review. But everything looks good now.

comment:17 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 in reply to: ↑ 16 Changed 8 years ago by rbeezer

Replying to ddrake:

Oops, I should have caught those two little errors in my first review.

Oops, they never should have been there in the first place. Mea culpa.

Thanks, Dan and Jeroen, for seeing this one through.

Note: See TracTickets for help on using tickets.