Opened 5 years ago
Closed 5 years ago
#24468 closed defect (fixed)
Fix unpickling old pickles of MeatAxe matrices
Reported by:  Simon King  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  linear algebra  Keywords:  
Cc:  JeanPhilippe Labbé, Travis Scrimshaw, Vincent Delecroix  Merged in:  
Authors:  Simon King  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  1bc5815 (Commits, GitHub, GitLab)  Commit:  1bc5815e7121fd1b4d6f58c300e7eabc0148e367 
Dependencies:  Stopgaps: 
Description (last modified by )
By #23706, each matrix space MS prescribes an implementation. It is important that each matrix whose parent is MS uses that implementation, for otherwise comparison breaks, as in the following example:
sage: from sage.matrix.matrix_gfpn_dense import Matrix_gfpn_dense sage: MS = MatrixSpace(GF(2), 1, 2) sage: M = Matrix_gfpn_dense(MS, [1,1]) sage: MS2 = MatrixSpace(GF(2), 2, 2) sage: M2 = Matrix_gfpn_dense(MS2, [1,1,1,1]) sage: M == M[0:1] True sage: M == M2[0:1] False sage: M.parent() Full MatrixSpace of 1 by 2 dense matrices over Finite Field of size 2 sage: M2[0:1].parent() Full MatrixSpace of 1 by 2 dense matrices over Finite Field of size 2 (using Matrix_gfpn_dense) sage: _ == __ False
Of course, the above example is a wrong usage. However, the helper function mtx_unpickle
would incorrectly create a Matrix_gfpn_dense
in a generic matrix space. The (new) purpose of this ticket is to fix it...
Change History (20)
comment:1 followup: 3 Changed 5 years ago by
comment:2 Changed 5 years ago by
I guess the matrix
constructor should be provided with an implementation
keyword, too.
Two matrix spaces with the same base ring, number of rows and number of columns are clearly canonically isomorphic, even if they have different implementations. Thus, there should be a coercion in at least one direction. That's the same in other parts of Sage, e.g., polynomials:
sage: R1 = PolynomialRing(ZZ,'x',implementation='NTL') sage: R2 = PolynomialRing(ZZ,'x',implementation='FLINT') sage: R3 = PolynomialRing(ZZ,'x',implementation='singular') sage: R3 Multivariate Polynomial Ring in x over Integer Ring sage: R1 Univariate Polynomial Ring in x over Integer Ring (using NTL) sage: R1.has_coerce_map_from(R2) False sage: R2.has_coerce_map_from(R1) True sage: R3.has_coerce_map_from(R1) True sage: R3.has_coerce_map_from(R2) True sage: R1.has_coerce_map_from(R3) False sage: R2.has_coerce_map_from(R3) False
comment:3 Changed 5 years ago by
Replying to SimonKing:
Related problem:
mtx_unpickle
hardcodes the usage of a matrix space with default implementation, even when the matrixtobeunpickled has a nondefault implementation.
To be fair, that statement only holds for matrices that have been created with an old version of my group cohomology spkg (that's why I care...).
So, I suggest one should deal with this ticket as follows:
 modify mtx_unpickle(), so that very old pickles are automatically unpickled with a matrix space that matches the implementation.
 For newer Matrix_gfpn_dense pickles (those which store the matrix space, rather than only the field size and the row/column numbers), nothing needs to change in mtx_unpickle.
 The creation of matrices as demonstrated in the ticket description shouldn't be supported: Matrices are to be constructed via the matrix space or some constructor.
comment:4 Changed 5 years ago by
Dependencies:  → #24359 

Description:  modified (diff) 
comment:5 Changed 5 years ago by
Branch:  → u/SimonKing/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces 

comment:6 Changed 5 years ago by
Authors:  → Simon King 

Commit:  → 423271a9a67bf66e5844f056bb4ba8976f01c2b2 
Status:  new → needs_review 
I added #24359 as a dependency, because it affects some tests happening in the file that I modified.
The new test passes, and I suppose it is ready for review!
Last 10 new commits:
228bb6f  Explain how to initialise the use of meataxe library functions

86d14cd  Fix SPKG.txt for meataxe

3c47bcf  quote SPKG_LOCAL in meataxe spkginstall

0fb61b4  Use the sdh_* functions in meataxe spkginstall

8c0216b  Updated meataxe checksums

6339d94  Remove unneeded lines from meataxe's spkginstall

13e2c76  Fix makefile syntax in meataxe tarball

94fafb7  Fixing meataxe for systems with unsigned 'char'

7ad7b08  Fix a race condition in meataxe test suite

423271a  Merge branch 't/24359/turn_meataxe_into_a_dynamic_library' into t/24468/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces

comment:7 Changed 5 years ago by
Commit:  423271a9a67bf66e5844f056bb4ba8976f01c2b2 → f910d1c2c40688dd71a97aeb4025d2ccc0178ef6 

Branch pushed to git repo; I updated commit sha1. New commits:
f910d1c  Use compatible implementation for MatrixSpace in mtx_unpickle

comment:8 Changed 5 years ago by
Oops, I forgot to commit the actual changes for this ticket.
Done now.
New commits:
f910d1c  Use compatible implementation for MatrixSpace in mtx_unpickle

comment:9 followups: 10 12 Changed 5 years ago by
You should never initialize by yourself M = Matrix_gfpn_dense(MS, [1,1])
. This might lead to crash. Each parent has its own element class. Your third point "If a matrix space MS has a matrix class doesn't match the class of a matrix M, ..." should never happen.
As you noticed, the comparison problem is due to the fact that I forbade coercions (it was simpler that way).
sage: M1 = MatrixSpace(GF(2), 1, implementation='generic') sage: M2 = MatrixSpace(GF(2), 1, implementation='m4ri') sage: M1 == M2 False sage: M1.one() == M2.one() False
A reasonable possibility is to allow coercions from matrix space with alternative implementation to the one with the default implementation. But this will not help for comparing matrices with two alternative implementations.
comment:10 followup: 11 Changed 5 years ago by
Replying to vdelecroix:
You should never initialize by yourself
M = Matrix_gfpn_dense(MS, [1,1])
. This might lead to crash. Each parent has its own element class. Your third point "If a matrix space MS has a matrix class doesn't match the class of a matrix M, ..." should never happen.
Fair enough. But I think we should at least make sure (because it is easy!) that old pickles unpickle according to the new standard. And that's what commit:f910d1c does.
comment:11 Changed 5 years ago by
Replying to SimonKing:
Replying to vdelecroix:
You should never initialize by yourself
M = Matrix_gfpn_dense(MS, [1,1])
. This might lead to crash. Each parent has its own element class. Your third point "If a matrix space MS has a matrix class doesn't match the class of a matrix M, ..." should never happen.Fair enough. But I think we should at least make sure (because it is easy!) that old pickles unpickle according to the new standard. And that's what commit:f910d1c does.
Definitely!
What do you think about coercion? (certainly for later ticket)
comment:12 followup: 13 Changed 5 years ago by
Replying to vdelecroix:
A reasonable possibility is to allow coercions from matrix space with alternative implementation to the one with the default implementation. But this will not help for comparing matrices with two alternative implementations.
It would, if we were including the implementation
in the matrix space construction functor, and would let merge()
always return the default implementation, unless the two tobemerged matrix space construction functors have both the same nondefault implementation.
comment:13 Changed 5 years ago by
Replying to SimonKing:
Replying to vdelecroix:
A reasonable possibility is to allow coercions from matrix space with alternative implementation to the one with the default implementation. But this will not help for comparing matrices with two alternative implementations.
It would, if we were including the
implementation
in the matrix space construction functor, and would letmerge()
always return the default implementation, unless the two tobemerged matrix space construction functors have both the same nondefault implementation.
Nice. I like it. I believe it is this way for polynomial rings. Though, for matrix spaces the default implementation is subtle as it depends on the presence of optional packages (i.e. meataxe
).
comment:14 Changed 5 years ago by
Branch:  u/SimonKing/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces → u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces 

comment:15 Changed 5 years ago by
Commit:  f910d1c2c40688dd71a97aeb4025d2ccc0178ef6 → 1bc5815e7121fd1b4d6f58c300e7eabc0148e367 

Dependencies:  #24359 
comment:16 followup: 17 Changed 5 years ago by
Status:  needs_review → needs_info 

This branch doesn't really correspond to the topic of the ticket. What am I missing?
comment:17 Changed 5 years ago by
Replying to jdemeyer:
This branch doesn't really correspond to the topic of the ticket. What am I missing?
When I created the ticket, I thought that comparison of matrices was broken. Then I realised that the example from the ticket description is a user error. But I also realised that there is a backward incompatibility, in the sense of "old pickles of meataxe matrices won't unpickle in the right matrix space". And that's what is being fixed.
I'll change the ticket title and description accordingly.
comment:18 Changed 5 years ago by
Description:  modified (diff) 

Status:  needs_info → needs_review 
Summary:  Fix comparison of matrices that live in equivalent matrix spaces → Fix unpickling old pickles of MeatAxe matrices 
comment:19 Changed 5 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → positive_review 
comment:20 Changed 5 years ago by
Branch:  u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces → 1bc5815e7121fd1b4d6f58c300e7eabc0148e367 

Resolution:  → fixed 
Status:  positive_review → closed 
Related problem:
mtx_unpickle
hardcodes the usage of a matrix space with default implementation, even when the matrixtobeunpickled has a nondefault implementation.