Opened 12 years ago

Closed 5 weeks ago

#7989 closed defect (fixed)

Minpoly doesn't work for all matrices

Reported by: jason Owned by: was
Priority: major Milestone: sage-9.7
Component: linear algebra Keywords:
Cc: kcrisman, rbeezer, mhansen, hedtke, tscrim Merged in:
Authors: Jason Grout, Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 329d472 (Commits, GitHub, GitLab) Commit: 329d4724ceb242e197af9a7a5aa4693d75a2308a
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

Right now, not all matrices can compute minpolys. This patch exposes these matrices.

Attachments (1)

trac-7989-minpoly-test.patch (1.1 KB) - added by jason 12 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 12 years ago by jason

  • Description modified (diff)

Changed 12 years ago by jason

comment:2 Changed 12 years ago by jason

Here are the bugs (doctests that fail) this patch exposes:

	sage -t  "4.3.1.rc0/devel/sage-main/sage/matrix/matrix1.pyx"
	sage -t  "4.3.1.rc0/devel/sage-main/sage/matrix/matrix_dense.pyx"
	sage -t  "4.3.1.rc0/devel/sage-main/sage/matrix/matrix_generic_dense.pyx"
	sage -t  "4.3.1.rc0/devel/sage-main/sage/matrix/matrix_integer_2x2.pyx"

comment:3 Changed 12 years ago by jason

  • Authors set to Jason Grout
  • Cc mhansen added

comment:4 Changed 12 years ago by jason

  • Status changed from new to needs_work

comment:5 Changed 11 years ago by hedtke

  • Cc hedtke added

Sorry, I don't understand the reason for this function. What is the idea behind it?

comment:6 Changed 11 years ago by was

This is a great patch. I can't believe this has got ignored for the last 1.5 years.

Regarding what needs to be fixed, the test failure you claim for matrix_integer_2x2 is:

deep:sage wstein$ sage -t matrix/matrix_integer_2x2.pyx
sage -t  "matrix/matrix_integer_2x2.pyx"                    
**********************************************************************
File "/Users/wstein/sage/install/sage-4.7.1.rc0/devel/sage-main/sage/matrix/matrix_integer_2x2.pyx", line 101:
    sage: TestSuite(m).run()
Expected nothing
Got:
    Failure in _test_minpoly:
    Traceback (most recent call last):
      File "/Users/wstein/sage/install/current/local/lib/python/site-packages/sage/misc/sage_unittest.py", line 275, in run
        test_method(tester = tester)
      File "matrix2.pyx", line 1302, in sage.matrix.matrix2.Matrix._test_minpoly (sage/matrix/matrix2.c:8933)
      File "polynomial_element.pyx", line 358, in sage.rings.polynomial.polynomial_element.Polynomial.subs (sage/rings/polynomial/polynomial_element.c:5624)
      File "polynomial_element.pyx", line 557, in sage.rings.polynomial.polynomial_element.Polynomial.__call__ (sage/rings/polynomial/polynomial_element.c:5819)
      File "polynomial_element.pyx", line 638, in sage.rings.polynomial.polynomial_element.Polynomial.__call__ (sage/rings/polynomial/polynomial_element.c:7244)
      File "element.pyx", line 1302, in sage.structure.element.RingElement.__add__ (sage/structure/element.c:11504)
      File "coerce.pyx", line 766, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:7337)
    TypeError: unsupported operand parent(s) for '+': 'Space of 2x2 integer matrices' and 'Integer Ring'
    ------------------------------------------------------------
    The following tests failed: _test_minpoly
**********************************************************************
1 items had failures:
   1 of   6 in __main__.example_7
***Test Failed*** 1 failures.
For whitespace errors, see the file /Users/wstein/.sage//tmp/.doctest_matrix_integer_2x2.py
	 [4.6 s]

This is because substitution isn't even implemented for that class:

sage: MS = sage.matrix.matrix_integer_2x2.MatrixSpace_ZZ_2x2()
sage: a = MS([1,2,3,4])
sage: a.minpoly()
x^2 - 5*x - 2
sage: a.minpoly()(a)
---------------------------------------------------------------------------
TypeError     

Also, if one adds this for minpoly, it would make sense to also add _test_charpoly.

-- William

comment:7 follow-up: Changed 11 years ago by hedtke

OK. Works for me, too. Is it favored that we include a test that purposely fails?

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

Replying to hedtke:

OK. Works for me, too. Is it favored that we include a test that purposely fails?

I think the idea is to include the test-suite checking and add the fixes needed to make the tests pass.

comment:9 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:10 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:11 Changed 8 years ago by mmezzarobba

sage-6.2.beta4:

sage -t src/sage/matrix/matrix_generic_dense.pyx  # 1 doctest failed
sage -t src/sage/matrix/matrix_dense.pyx  # 1 doctest failed

comment:12 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/minpoly_doesn_t_work_for_all_matrices

comment:15 Changed 6 weeks ago by chapoton

  • Branch changed from u/jdemeyer/minpoly_doesn_t_work_for_all_matrices to public/ticket/7989
  • Commit set to 08701a4701f1acad04a8d9663f103540c71a7cc4

New commits:

08701a4test that minpoly works

comment:16 Changed 6 weeks ago by git

  • Commit changed from 08701a4701f1acad04a8d9663f103540c71a7cc4 to ce364f737b21c2131710bbfd199936b520dc2198

Branch pushed to git repo; I updated commit sha1. New commits:

ce364f7only test minpoly for exact rings

comment:17 Changed 6 weeks ago by git

  • Commit changed from ce364f737b21c2131710bbfd199936b520dc2198 to 216d945af1a918c569191473cb1890733769318d

Branch pushed to git repo; I updated commit sha1. New commits:

216d945use Gap for minpoly of Gap matrices

comment:18 Changed 6 weeks ago by git

  • Commit changed from 216d945af1a918c569191473cb1890733769318d to 9f476b44608a2acacf7e86bfec99a754fa80f796

Branch pushed to git repo; I updated commit sha1. New commits:

9f476b4add doc, skip test

comment:19 Changed 6 weeks ago by git

  • Commit changed from 9f476b44608a2acacf7e86bfec99a754fa80f796 to 329d4724ceb242e197af9a7a5aa4693d75a2308a

Branch pushed to git repo; I updated commit sha1. New commits:

329d472skip another test

comment:20 Changed 6 weeks ago by chapoton

  • Authors changed from Jason Grout to Jason Grout, Frédéric Chapoton
  • Milestone changed from sage-6.4 to sage-9.7
  • Status changed from needs_work to needs_review

comment:21 Changed 6 weeks ago by chapoton

  • Cc tscrim added

green patchbot, so please review

comment:22 Changed 6 weeks ago by tscrim

  • Description modified (diff)
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:23 Changed 5 weeks ago by vbraun

  • Branch changed from public/ticket/7989 to 329d4724ceb242e197af9a7a5aa4693d75a2308a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.