#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 )
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:
Attachments (3)
Change History (21)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 9 years ago by
- Reviewers set to Dan Drake
- Status changed from needs_review to positive_review
comment:3 in reply to: ↑ 2 Changed 9 years ago by
Replying to ddrake:
Thanks for your help with all this linear algebra code.
comment:4 Changed 9 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
comment:5 Changed 8 years ago by
- 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
comment:6 follow-up: ↓ 7 Changed 8 years ago by
- 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
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
- Merged in set to sage-4.7.1.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:9 follow-up: ↓ 10 Changed 8 years ago by
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
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 secondself
should beother
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: ↓ 12 Changed 8 years ago by
Should I reopen the ticket so you can make the changes here?
comment:12 in reply to: ↑ 11 Changed 8 years ago by
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
Changed 8 years ago by
comment:13 Changed 8 years ago by
- 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
- Merged in sage-4.7.1.alpha2 deleted
- Resolution fixed deleted
- Status changed from closed to new
comment:15 Changed 8 years ago by
- Status changed from new to needs_review
comment:16 follow-up: ↓ 18 Changed 8 years ago by
- 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
- 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
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.
This is very nice work. Applies and passes doctests in 4.6.2 and 4.7.rc0. Positive review.