Segfault in solve_left for sparse matrices over ZZ

[davidloeffler]

Calling solve_left on a sparse integer matrix results in an instant segfault:

sage: A = identity_matrix(ZZ, 2, sparse=True)
sage: B = identity_matrix(ZZ, 2, sparse=True)
sage: A.solve_left(B)
/storage/masiao/sage-5.0.beta8/spkg/bin/sage: line 308:  7843 Segmentation fault      sage-ipython "$@" -i

[dsm]

We can push the bug a bit farther back:

sage: A = identity_matrix(ZZ, 2, sparse=True)
sage: B = identity_matrix(ZZ, 2, sparse=True)
sage: A.augment(B)
[1 0 1 0]
[0 1 0 1]
sage: A.change_ring(QQ).augment(B.change_ring(QQ))
[1 0 1 0]
[0 1 0 1]
sage: A.change_ring(QQ).augment(B)
[  1   0 1/0   0]
[  0   1   0 1/0]

1/0 doesn't look right..

[dsm]

Ah, okay. I think I see what's going on. In matrix_sparse.pyx's augment:

        cdef Matrix_sparse other = right.sparse_matrix()

    [...]

        if not (self._base_ring is right.base_ring()):
            right = right.change_ring(self._base_ring)

attemps to change the right base ring to agree, but during the insertion process, it still uses the old unchanged one:

        for i, j in right.nonzero_positions(copy=False):
            Z.set_unsafe(i, j + self._ncols, other.get_unsafe(i,j))

If we instead use

            other = other.change_ring(self._base_ring)

then everything seems to work:

sage: z = A.change_ring(QQ).augment(B)
[skipping debugging output]
sage: z
[1 0 1 0]
[0 1 0 1]

and

sage: A.solve_left(B)
[1 0]
[0 1]
sage: parent(_)
Full MatrixSpace of 2 by 2 sparse matrices over Rational Field

So I think the fix is a one-liner. Any takers?

[was]

Any takes?

I'll post a patch if you'll review it :-)

[dsm]

Deal. Should probably add tests both in augment for the incompatible-ring bug and in solve_left for the original.

[dsm]

Although since uses Z._subdivide_on_augment(self, right) uses right too, maybe it's a two-liner. :⁾

[dsm]

Running test suite now, but it looks fine. Don't forget that we have the new ":trac:12689" markup now, though!

[was]

Replying to dsm:

Running test suite now, but it looks fine. Don't forget that we have the new ":trac:12689" markup now, though!

Cool. I've never heard of it.

[dsm]

Patch applies against 5.0.beta8 (and 4.8) and runs; fixes the OP's symptom; has a doctest; doesn't introduce new doctest failures (I have quite a few on beta8 but they came before, and it works cleanly against 4.8); and it's clear what the bug was and why this patch fixes it.

Positive review.