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: sage-8.2
Component: linear algebra Keywords:
Cc: Jean-Philippe 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:

Status badges

Description (last modified by Simon King)

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 Changed 5 years ago by Simon King

Related problem: mtx_unpickle hard-codes the usage of a matrix space with default implementation, even when the matrix-to-be-unpickled has a non-default implementation.

comment:2 Changed 5 years ago by Simon King

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 in reply to:  1 Changed 5 years ago by Simon King

Replying to SimonKing:

Related problem: mtx_unpickle hard-codes the usage of a matrix space with default implementation, even when the matrix-to-be-unpickled has a non-default 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 Simon King

Dependencies: #24359
Description: modified (diff)

comment:5 Changed 5 years ago by Simon King

Branch: u/SimonKing/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces

comment:6 Changed 5 years ago by Simon King

Authors: Simon King
Commit: 423271a9a67bf66e5844f056bb4ba8976f01c2b2
Status: newneeds_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:

228bb6fExplain how to initialise the use of meataxe library functions
86d14cdFix SPKG.txt for meataxe
3c47bcfquote SPKG_LOCAL in meataxe spkg-install
0fb61b4Use the sdh_* functions in meataxe spkg-install
8c0216bUpdated meataxe checksums
6339d94Remove unneeded lines from meataxe's spkg-install
13e2c76Fix makefile syntax in meataxe tarball
94fafb7Fixing meataxe for systems with unsigned 'char'
7ad7b08Fix a race condition in meataxe test suite
423271aMerge 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 git

Commit: 423271a9a67bf66e5844f056bb4ba8976f01c2b2f910d1c2c40688dd71a97aeb4025d2ccc0178ef6

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

f910d1cUse compatible implementation for MatrixSpace in mtx_unpickle

comment:8 Changed 5 years ago by Simon King

Oops, I forgot to commit the actual changes for this ticket.

Done now.


New commits:

f910d1cUse compatible implementation for MatrixSpace in mtx_unpickle

comment:9 Changed 5 years ago by Vincent Delecroix

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 in reply to:  9 ; Changed 5 years ago by Simon King

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 in reply to:  10 Changed 5 years ago by Vincent Delecroix

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 in reply to:  9 ; Changed 5 years ago by Simon King

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 to-be-merged matrix space construction functors have both the same non-default implementation.

comment:13 in reply to:  12 Changed 5 years ago by Vincent Delecroix

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 let merge() always return the default implementation, unless the two to-be-merged matrix space construction functors have both the same non-default 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 Jeroen Demeyer

Branch: u/SimonKing/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spacesu/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces

comment:15 Changed 5 years ago by Jeroen Demeyer

Commit: f910d1c2c40688dd71a97aeb4025d2ccc0178ef61bc5815e7121fd1b4d6f58c300e7eabc0148e367
Dependencies: #24359

Removed the dependencies, mainly to deal with the fact that #24359 was force-pushed. Feel free to put back whatever dependency you need, although the current branch doesn't have any dependencies as far as I can tell.


New commits:

1bc5815Use compatible implementation for MatrixSpace in mtx_unpickle

comment:16 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_info

This branch doesn't really correspond to the topic of the ticket. What am I missing?

comment:17 in reply to:  16 Changed 5 years ago by Simon King

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 Simon King

Description: modified (diff)
Status: needs_infoneeds_review
Summary: Fix comparison of matrices that live in equivalent matrix spacesFix unpickling old pickles of MeatAxe matrices

comment:19 Changed 5 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer
Status: needs_reviewpositive_review

comment:20 Changed 5 years ago by Volker Braun

Branch: u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces1bc5815e7121fd1b4d6f58c300e7eabc0148e367
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.