Opened 8 years ago

Closed 7 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:

Status badges

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)

trac_14508_matrix_jordan_form.patch (2.0 KB) - added by itolkov 8 years ago.
trac_14508-possible-improvement-dg.patch (3.7 KB) - added by darij 8 years ago.
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?

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by itolkov

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by itolkov

  • Status changed from needs_review to needs_work

Now that I'm at it, should probably fix a related issue.

sage: M=matrix([[0,1,0],[0,0,1],[1,0,0]])
sage: M.jordan_form(CyclotomicField(3))
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-2-32d8bd4f220b> in <module>()
----> 1 M.jordan_form(CyclotomicField(Integer(3)))

/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:44368)()

RuntimeError: Some eigenvalue does not exist in Integer Ring.

comment:3 Changed 8 years ago by itolkov

  • Status changed from needs_work to needs_review

comment:4 Changed 8 years ago by novoselt

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 8 years ago by itolkov

How about this?

Changed 8 years ago by itolkov

comment:6 Changed 8 years ago by ncohen

  • 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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 8 years ago by darij

  • Cc tscrim nbruin added

Changed 8 years ago by darij

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 darij

  • Status changed from needs_info to needs_review

comment:10 Changed 7 years ago by darij

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 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:12 Changed 7 years ago by sstarosta

  • Cc sstarosta added

comment:13 Changed 7 years ago by ncohen

  • Authors changed from Igor Tolkov to Igor Tolkov, Darij Grinberg
  • Branch set to public/14508
  • Commit set to 1019449de346204f8a4ee5c1f55d60d730250c4b
  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

HellooooooooO !!

Sorry for not having answered earlier, I must have missed the emails :-/

Anyway it looks good to me. Positive review, with a brand new git branch.

Nathann


New commits:

d2c220dTrac 14508: bugfix in jordan_form
1019449trac #14508: better now?

comment:14 Changed 7 years ago by darij

Thank you! Nothing like a positive review from Nathann when it comes to potential slowdown issues.

comment:15 Changed 7 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.