Opened 5 years ago

Closed 5 years ago

# Fix unpickling old pickles of MeatAxe matrices

Reported by: Owned by: Simon King major sage-8.2 linear algebra Jean-Philippe Labbé, Travis Scrimshaw, Vincent Delecroix Simon King Jeroen Demeyer N/A 1bc5815 1bc5815e7121fd1b4d6f58c300e7eabc0148e367

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...

### comment:1 follow-up:  3 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

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 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 → 423271a9a67bf66e5844f056bb4ba8976f01c2b2 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 spkg-install` ​0fb61b4 `Use the sdh_* functions in meataxe spkg-install` ​8c0216b `Updated meataxe checksums` ​6339d94 `Remove unneeded lines from meataxe's spkg-install` ​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 git

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

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 follow-ups:  10  12 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 ; follow-up:  11 Changed 5 years ago by Simon King

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

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 ; follow-up:  13 Changed 5 years ago by Simon King

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

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_spaces → u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces

### comment:15 Changed 5 years ago by Jeroen Demeyer

Commit: f910d1c2c40688dd71a97aeb4025d2ccc0178ef6 → 1bc5815e7121fd1b4d6f58c300e7eabc0148e367 #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:

 ​1bc5815 `Use compatible implementation for MatrixSpace in mtx_unpickle`

### comment:16 follow-up:  17 Changed 5 years ago by Jeroen Demeyer

Status: needs_review → needs_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

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) needs_info → needs_review Fix comparison of matrices that live in equivalent matrix spaces → Fix unpickling old pickles of MeatAxe matrices

### comment:19 Changed 5 years ago by Jeroen Demeyer

Reviewers: → Jeroen Demeyer needs_review → positive_review

### comment:20 Changed 5 years ago by Volker Braun

Branch: u/jdemeyer/fix_comparison_of_matrices_that_live_in_equivalent_matrix_spaces → 1bc5815e7121fd1b4d6f58c300e7eabc0148e367 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.