Opened 4 years ago

Closed 4 years ago

#24445 closed defect (fixed)

Allow several implementations in random matrix

Reported by: SimonKing Owned by:
Priority: blocker Milestone: sage-8.2
Component: linear algebra Keywords:
Cc: jipilab, tscrim, vdelecroix Merged in:
Authors: Simon King Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 652ca07 (Commits, GitHub, GitLab) Commit: 652ca074f02c743bd2693bb07909a866663b828e
Dependencies: Stopgaps:

Status badges

Description

With the optional meataxe package installed, I get 

sage: K.<a>=FiniteField(3^2)
sage: MS = MatrixSpace(K, 2, 5, implementation="generic")
sage: type(MS.random_element())
<type 'sage.matrix.matrix_generic_dense.Matrix_generic_dense'>

which is good, but 

sage: type(random_matrix(K, 2, 5, implementation="generic"))
<type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>

which is bad.

So, the random_matrix constructor should take into account what was contributed in #23706.

Change History (14)

comment:1 Changed 4 years ago by SimonKing

  • Branch set to u/SimonKing/allow_several_implementations_in_random_matrix

comment:2 Changed 4 years ago by git

  • Commit set to 571c12fa24badd695978e8ecad9152aa4a9d4847

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

​571c12fFix changed random_matrix output in tests

comment:3 Changed 4 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

comment:4 follow-up: Changed 4 years ago by vdelecroix

Could you change the following doctest (matrix/special.py line 403) 

The default implementation of :meth:`~sage.matrix.matrix2.randomize` relies
on the ``random_element()`` method for the base ring.  The ``density`` and
``sparse`` keywords behave as described above. Since we have a different
randomisation when using the optional meataxe package, we have to make sure
that we use the default implementation in this test::

    sage: K.<a>=FiniteField(3^2)
    sage: from sage.matrix.matrix_generic_dense import Matrix_generic_dense
    sage: MS = MatrixSpace(K, 2, 5)
    sage: MS._MatrixSpace__matrix_class = Matrix_generic_dense
    sage: random_matrix(K, 2, 5)
    [      1       a       1 2*a + 1       2]
    [    2*a   a + 2       0       2       1]

It could now be much simpler without any special hack.

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Could you change the following doctest (matrix/special.py line 403)

The first commit does this: 

@@ -407,10 +419,7 @@ def random_matrix(ring, nrows, ncols=None, algorithm='randomize', *args, **kwds)
     that we use the default implementation in this test::
 
         sage: K.<a>=FiniteField(3^2)
-        sage: from sage.matrix.matrix_generic_dense import Matrix_generic_dense
-        sage: MS = MatrixSpace(K, 2, 5)
-        sage: MS._MatrixSpace__matrix_class = Matrix_generic_dense
-        sage: random_matrix(K, 2, 5)
+        sage: random_matrix(K, 2, 5, implementation='generic')
         [      1       a       1 2*a + 1       2]
         [    2*a   a + 2       0       2       1]

So, I did change that test. Actually the whole purpose of this ticket was to fix this test (that would fail because of #23706 when meataxe is installed).

comment:6 Changed 4 years ago by vdelecroix

Then I think that I should close #24449 as duplicate as Jeroen suggested.

comment:7 Changed 4 years ago by jdemeyer

  • Priority changed from major to blocker

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

  • Status changed from needs_review to needs_work

Doctest failure in src/sage/misc/lazy_import.pyx

comment:9 Changed 4 years ago by git

  • Commit changed from 571c12fa24badd695978e8ecad9152aa4a9d4847 to 652ca074f02c743bd2693bb07909a866663b828e

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

​652ca07Fix a doctest in lazy_import

comment:10 in reply to: ↑ 8 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Doctest failure in src/sage/misc/lazy_import.pyx

Done

comment:11 follow-up: Changed 4 years ago by dimpase

It seems there should be doctests/examples showing the use of MeatAxe package as a backend.

comment:12 in reply to: ↑ 11 Changed 4 years ago by SimonKing

Replying to dimpase:

It seems there should be doctests/examples showing the use of MeatAxe package as a backend.

See basically all tests in sage.matrix.matrix_gfpn_dense

comment:13 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:14 Changed 4 years ago by vbraun

  • Branch changed from u/SimonKing/allow_several_implementations_in_random_matrix to 652ca074f02c743bd2693bb07909a866663b828e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.