Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong results in vector matrix multiplication #23576

Open
simonbrandhorst opened this issue Aug 3, 2017 · 10 comments
Open

Wrong results in vector matrix multiplication #23576

simonbrandhorst opened this issue Aug 3, 2017 · 10 comments

Comments

@simonbrandhorst
Copy link

sage: M = FreeModule(ZZ,1).span([vector([1/3])])
sage: x = M([1/3])
sage: x
(1/3)
sage: A = Matrix(QQ,[1])
sage: x*A , A*x
((1/3), (1/3))

This is good.

sage: B = Matrix(ZZ,[1])
sage: x*B , B*x
((1), (1/3))

The first entry is clearly wrong.
Somehow the first product is carried out in a wrong basis representation for x.

CC: @tscrim

Component: linear algebra

Keywords: sd91

Branch/Commit: u/roed/vmat_action @ 6021daa

Issue created by migration from https://trac.sagemath.org/ticket/23576

@simonbrandhorst
Copy link
Author

Changed keywords from none to sd91

@roed314
Copy link
Contributor

roed314 commented Oct 2, 2017

comment:2

The problem appears in line 1122 of sage.matrix.matrix_integer_dense.pyx, inside the function cdef _vector_times_matrix_(self, Vector v):

w = <Vector_integer_dense> v

If you change that to

w = <Vector_integer_dense?> v

you'll get an error, because v is a Vector_rational_dense, and if you change it to

try:
    w = <Vector_integer_dense?> v
except TypeError:
    return Matrix_dense._vector_times_matrix_(self, v)

you get (1/3).

This function is replacing a more general implementation in sage.matrix.matrix0, but it only replaces _vector_times_matrix_, not _matrix_times_vector_, thus the disparity based on whether B is acting on the left or right.

I don't know if the try/except block is the right answer. I'll think about it....

@roed314
Copy link
Contributor

roed314 commented Oct 2, 2017

comment:3

There's maybe a separate issue in sage.matrix.action, since it detects the base ring of the result by checking the base ring of the inputs. But here x and B both have base ring ZZ, and indeed

sage: from sage.structure.element import coercion_model
sage: from operator import mul
sage: coercion_model.explain(x.parent(), B.parent(), mul)
Action discovered.
    Right action by Full MatrixSpace of 1 by 1 dense matrices over Integer Ring on Free module of degree 1 and rank 1 over Integer Ring
    Echelon basis matrix:
    [1/3]
Result lives in Ambient free module of rank 1 over the principal ideal domain Integer Ring
Ambient free module of rank 1 over the principal ideal domain Integer Ring

The result should probably have rational entries....

@roed314
Copy link
Contributor

roed314 commented Oct 4, 2017

Branch: u/roed/vmat_action

@roed314
Copy link
Contributor

roed314 commented Oct 4, 2017

comment:5

There are segfaults in the new implementation that multiplies a non-square integral matrix times a rational vector. Still working on them...


New commits:

6021daaCorrect base ring for matrices acting on submodules, implement multiplication for Vector_rational_dense times an integral matrix.

@roed314
Copy link
Contributor

roed314 commented Oct 4, 2017

Commit: 6021daa

@simonbrandhorst
Copy link
Author

comment:6

ping :)
Maybe #24031 is related to this? After all that bug occurs for vectors as well.
Does this one for matrices too?

@mkoeppe
Copy link
Member

mkoeppe commented Jul 4, 2020

comment:7

Still unresolved as of 9.2.beta2

@mkoeppe mkoeppe modified the milestones: sage-8.1, sage-9.2 Jul 4, 2020
@slel
Copy link
Member

slel commented Aug 31, 2020

comment:8

There is a possibility that #28544 will solve this,
see comment 10 there.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@mkoeppe
Copy link
Member

mkoeppe commented May 10, 2021

comment:10

Moving to 9.4, as 9.3 has been released.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 May 3, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants