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

More matrix features to MathObject matrices #1076

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

pstaabp
Copy link
Member

@pstaabp pstaabp commented May 17, 2024

This builds on #1012 and includes

  • new methods for setting elements, removing columns and rows and a submatrix.
  • A test suite for nearly all of the methods in the module.
  • updated POD.

@pstaabp pstaabp force-pushed the more-matrix-features branch from da3b3bf to 3df446c Compare May 18, 2024 10:53
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
@Alex-Jordan
Copy link
Contributor

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.

lib/Value/Matrix.pm Outdated Show resolved Hide resolved
lib/Value/Matrix.pm Outdated Show resolved Hide resolved
$A->subMatrix([3,1,2],[1,4,2]); # returns Matrix([9,12,10],[1,4,2],[5,8,6]);
=cut

sub subMatrix {
Copy link
Contributor

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]);

Copy link
Contributor

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.)

Copy link
Member Author

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.

Copy link
Member Author

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.

Comment on lines 1001 to 1005
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be

Suggested change
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.

Copy link
Member Author

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.

@drgrice1 drgrice1 deleted the branch openwebwork:develop August 6, 2024 19:46
@drgrice1 drgrice1 closed this Aug 6, 2024
@pstaabp pstaabp reopened this Aug 6, 2024
@pstaabp pstaabp changed the base branch from PG-2.19 to develop August 6, 2024 20:02
@pstaabp pstaabp force-pushed the more-matrix-features branch 2 times, most recently from 34fd8ed to 9ba941c Compare August 13, 2024 20:06
Also fix some documentation typos and clarifications.

u
@pstaabp pstaabp force-pushed the more-matrix-features branch from 9ba941c to c365efe Compare August 13, 2024 21:01

Information
$matrix->dimension: ARRAY
$matrix->value produces [[3,4,5],[1,3,4]] recursive array references of numbers (not MathObjects)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be incorrect. (It was incorrect before this PR.) $matrix->value produces an actual array, not an array reference. Its entries are either numbers (if $matrix was 1D) or are array references. I don't have a suggestion for how to succinctly write that in the POD.

row : MathObjectMatrix
column : MathObjectMatrix
element : Real or Complex value
$matrix->dimension: ARRAY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also incorrect (and was before). There is no dimension method. However there is dimensions, which returns an array of integers as you would expect. Usually two for what we think of normally as a matrix.

I think it would be goo if there were a dimension method though, that tells you if it is a 1D, 2D, 3D, etc matrix. It would just be evaluating $matrix->dimensions in a scalar context.

$A1->row(2); # returns the row Matrix [5,6,7,8]

=cut

sub row {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be convenient if row could take for its argument:

  • a whole number, returning a matrix that is one dimension less (in the 1D, 2D, 3D, etc sense) or just 1D if applied to a 1D matrix (as it currently does)
  • several whole numbers, returning a submatrix of the same dimension (2D, 3D)
  • an array ref of several whole numbers, equivalent to the above

$A1->column(2); # returns the column Matrix [[2],[6],[10]]

=cut

sub column {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment about row, the column method could take multiple whole numbers as arguments.

Also I am seeing what I think is undesirable behavior here with 1D matrices. For an nD matrix with n > 1, $matrix->column(1) is still an nD matrix. Good. But with a 1D matrix, $matrix->column(1) is a Real rather than a 1D matrix. I think for consistency it should still be a 1D matrix.

these need to be added:
row : MathObjectMatrix
column : MathObjectMatrix
element : Real or Complex value
Copy link
Contributor

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.)

@Alex-Jordan
Copy link
Contributor

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:

  1. Go through the current methods and (a) make them better if we see opportunity for that, and (b) correct their POD without moving POD around.
  2. Then reorganize the POD and provide/update a test suite for the existing methods. The test suite should be thorough with at least 1D, 2D, and 3D matrices.
  3. Then add new methods, with new POD, and new tests.

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.

@Alex-Jordan Alex-Jordan mentioned this pull request Feb 4, 2025
@pstaabp
Copy link
Member Author

pstaabp commented Feb 4, 2025

Would it make sense if we split this in two pieces?

  1. update the POD for existing methods and functionality
  2. Update the methods as you mentioned and with them any needed POD.

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

Successfully merging this pull request may close these issues.

4 participants