-
-
Notifications
You must be signed in to change notification settings - Fork 77
More matrix features to MathObject matrices #1076
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
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It did seem to work when I tested with 2D matrices though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of subMatrix only on 2D matrices, so will rework for other dimensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
This builds on #1012 and includes