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

It's possible to construct integer matrices that are not matrices #4885

Closed
james-d-mitchell opened this issue Apr 28, 2022 · 1 comment
Closed

Comments

@james-d-mitchell
Copy link
Contributor

As the title suggests, it's possible to create matrices over the integers, which are non-rectangular (i.e. not matrices), which think they are:

Observed behaviour

gap> Matrix(Integers, [[1, 1], [2]]);
<2x2-matrix over Integers>
gap> Display(last);
<2x2-matrix over Integers:
[[ 1, 1 ]
 [ 2 ]
]>
gap> x[2, 2] := 4;
4
gap> Display(x);
<2x2-matrix over Integers:
[[ 1, 1 ]
 [ 2, 4 ]
]>
gap> x[2, 5] := 0;
0
gap> x;
<2x2-matrix over Integers>
gap> Display(x);
<2x2-matrix over Integers:
[[ 1, 1 ]
 [ 2, 4,,, 0 ]
]>
gap>

Expected behaviour

gap> Matrix(Integers, [[1, 1], [2]]);
Error,

Perhaps this is the intended behaviour, but at the very least the following should probably occur.

gap> x[2, 5] := 0;
Error,
hulpke added a commit to hulpke/gap that referenced this issue Apr 28, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
hulpke added a commit to hulpke/gap that referenced this issue Apr 28, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
@fingolfin
Copy link
Member

These are indeed both bugs.

The assignment one should be relatively easy to fix. For the inversion bug, we should of course also fix it, but I am slightly concerned about performance; ideally, the function computing the inverse would already produce the error as soon as it computes a non-integer result. But right now, we'll compute the inverse by calling another GAP function, and then at the end we'll have to scan all entries to see if any is non-integer. That's annoyingly expensive... Then again, I think it is likely our inversion code is non-optimal for large integer matrices anyway, so I am probably "worrying" for nothing. Anyway, we should first make it correct, and then later can tune it as needed.

But before anybody delves too deeply into the IsPlistMatrixRep code to fix these, it would be great if my ancient PR #2973 was at least superficially reviewed and then merged, so that that work does not go to waste.

hulpke added a commit to hulpke/gap that referenced this issue Jun 20, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
hulpke added a commit to hulpke/gap that referenced this issue Jun 30, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
hulpke added a commit to hulpke/gap that referenced this issue Jul 1, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
hulpke added a commit to hulpke/gap that referenced this issue Jul 1, 2022
for square list of lists and otherwise bails out.
This resolves gap-system#4885
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants