More matrix features to MathObject matrices#1076
More matrix features to MathObject matrices#1076pstaabp wants to merge 2 commits intoopenwebwork:developfrom
Conversation
da3b3bf to
3df446c
Compare
|
I was looking at the POD since a lot of that is in the diff here, and I left some comments. But I'm going to pause because I think if you proofread the actual POD output you will see a lot of the same things that I'm seeing. I'll move on to testing the new tools now. |
| $A->subMatrix([3,1,2],[1,4,2]); # returns Matrix([9,12,10],[1,4,2],[5,8,6]); | ||
| =cut | ||
|
|
||
| sub subMatrix { |
There was a problem hiding this comment.
This has some issues as well. If I try it on a 1D matrix, I get an error:
$A = Matrix([ 1, 2, 3, 4 ]);
$B = $A->subMatrix([3]);
And if I try it on a 3D matrix, it is not returning what I would expect it to:
$A = Matrix([ [[1,2],[3,4]], [[5,6], [7,8]]]);
$B = $A->subMatrix([1], [1,2], [2]);
# $B seems to be what I would expect from subMatrix([1], [1,2], [1,2]);
There was a problem hiding this comment.
(It did seem to work when I tested with 2D matrices though.)
There was a problem hiding this comment.
I thought of subMatrix only on 2D matrices, so will rework for other dimensions.
There was a problem hiding this comment.
Rewrote this now. Should be working on non-2D matrices.
lib/Value/Matrix.pm
Outdated
| my $el = $self->{data}[ $ind->[0] - 1 ]; | ||
| for my $i (1 .. scalar(@$ind) - 1) { $el = $el->{data}[ $ind->[$i] - 1 ]; } | ||
|
|
||
| # update the value of $el | ||
| $el = Value::makeValue($value); |
There was a problem hiding this comment.
This needs to be
| my $el = $self->{data}[ $ind->[0] - 1 ]; | |
| for my $i (1 .. scalar(@$ind) - 1) { $el = $el->{data}[ $ind->[$i] - 1 ]; } | |
| # update the value of $el | |
| $el = Value::makeValue($value); | |
| my $el = \($self->{data}[ $ind->[0] - 1 ]); | |
| for my $i (1 .. scalar(@$ind) - 1) { $el = \($$el->{data}[ $ind->[$i] - 1 ]); } | |
| # update the value of $el | |
| $$el = Value::makeValue($value); |
my $el = \($self->{data}[ $ind->[0] - 1 ]);
for my $i (1 .. scalar(@$ind) - 1) { $el = \($$el->{data}[ $ind->[$i] - 1 ]); }
# update the value of $el
$$el = Value::makeValue($value);in order to actually mutate the matrix.
There was a problem hiding this comment.
Changed this, but odd that the test works for the previous version of the code as well as this one, however, I think the test needs some help.
34fd8ed to
9ba941c
Compare
Also fix some documentation typos and clarifications. u
9ba941c to
c365efe
Compare
| these need to be added: | ||
| row : MathObjectMatrix | ||
| column : MathObjectMatrix | ||
| element : Real or Complex value |
There was a problem hiding this comment.
See my other comment about column. Currently for a 1D matrix, $matrix->column(1) is a Real. I do think it should be changed to be a MathObject Matrix though.
With the element method, it is more complicated than this. If you have an nD matrix and you pass n whole numbers to element, then yes you get a Real (or Complex, I assume). But if you pass fewer than n whole numbers, you get a Matrix. (And if you pass more than n, you get null.)
|
See my recent comments just now. I feel like we need to work on this file in a more methodical manner, especially since there are things to fix that precede the new things here. I propose:
When I look at the diff for this PR, it's just too overwhelming to sort out what is just POD moving around and what is new content. |
|
Would it make sense if we split this in two pieces?
|
|
This is being rewritten using different PRs. The first one is #1197 which cleans up the POD and a second one will take some of the functionality in this PR. Once both are merged, we will close this one. |
|
We should catalog what is still here that needs to be added.
Did I miss anything? We should be careful to add them so that they work for a Matrix object of any degree. We should agree on what things like "removeRow" and "removeColumn" mean for degrees other than 2. |
|
I think the methods in |
This builds on #1012 and includes