Opened 9 years ago
Closed 8 years ago
#14508 closed defect (fixed)
jordan_form fails when base field specified
Reported by: | itolkov | Owned by: | itolkov |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | linear algebra | Keywords: | |
Cc: | tscrim, nbruin, sstarosta | Merged in: | |
Authors: | Igor Tolkov, Darij Grinberg | Reviewers: | Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | public/14508 (Commits, GitHub, GitLab) | Commit: | 1019449de346204f8a4ee5c1f55d60d730250c4b |
Dependencies: | Stopgaps: |
Description
sage: version() 'Sage Version 5.9.beta5, Release Date: 2013-04-11' sage: M=matrix([[0,1],[1,0]]); sage: M.jordan_form() [ 1| 0] [--+--] [ 0|-1] sage: M.jordan_form(RationalField()) --------------------------------------------------------------------------- UnboundLocalError Traceback (most recent call last) <ipython-input-6-3f73f89e1c74> in <module>() ----> 1 M.jordan_form(RationalField()) /usr/local/src/sage-5.9.beta5/local/lib/python2.7/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix.jordan_form (sage/matrix/matrix2.c:44243)() UnboundLocalError: local variable 'A' referenced before assignment
Fix coming shortly, as soon as I remember how to do that.
Attachments (2)
Change History (17)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:4 Changed 9 years ago by
Looks good, but the commit message should be more descriptive and you need to put the ticket number on a line of the test that checks that this bug was fixed, i.e. add a comment
... # Trac #14508
I failed to find a reference in the developer's guide, but I am pretty sure that was the consensus.
comment:5 Changed 9 years ago by
How about this?
Changed 9 years ago by
comment:6 Changed 9 years ago by
- Status changed from needs_review to needs_info
Helloooooo !
It looks good to me, but when you add
if base_ring.is_field(): A = self.change_ring(base_ring)
Then you make a copy of A
even if base_ring
is equal to
A.base_ring()
, aren't you ? Is that necessary ?
Nathann
comment:7 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:8 Changed 8 years ago by
- Cc tscrim nbruin added
Changed 8 years ago by
A is no longer basechanged if the base_ring keyword was None and the base ring of A is a field. is this what we want?
comment:9 Changed 8 years ago by
- Status changed from needs_info to needs_review
comment:10 Changed 8 years ago by
BTW, the change_ring
methods on all the matrix classes I've checked (dense integer matrices, dense rational matrices, dense generic matrices) start out (more or less -- sometimes, the ring is first checked for being a ring) by checking whether the ring to map into is already the base ring, and if so, do nothing or simply copy the matrix if it's mutable. So it shouldn't be a bad slowdown or something. And with my patch, it happens only if the user explicitly specifies the base_ring
variable. So is this still an issue?
comment:11 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:12 Changed 8 years ago by
- Cc sstarosta added
comment:13 Changed 8 years ago by
- Branch set to public/14508
- Commit set to 1019449de346204f8a4ee5c1f55d60d730250c4b
- Reviewers set to Nathann Cohen
- Status changed from needs_review to positive_review
comment:14 Changed 8 years ago by
Thank you! Nothing like a positive review from Nathann when it comes to potential slowdown issues.
comment:15 Changed 8 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
Now that I'm at it, should probably fix a related issue.