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

Develop 328 #338

Merged
merged 6 commits into from
Jun 3, 2018
Merged

Develop 328 #338

merged 6 commits into from
Jun 3, 2018

Conversation

mgage
Copy link
Member

@mgage mgage commented Dec 30, 2017

This pull requests is a cleaner version of pull request 328. Only the pod documents are modified.

@mgage
Copy link
Member Author

mgage commented Dec 30, 2017

This PR replaces PR#328 which added some files in addition to changing POD documentation. This PR should only change POD documentation for matrix files.

To test -- read through the documentation. You can request changes via the review mechanism if some portions of the documentation are incorrect or unclear.

lib/Matrix.pm Outdated
R($matrix) - return matrix R of the LR decomposition
PL($matrix) return
PR($matrix

Copy link
Member

Choose a reason for hiding this comment

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

Is there something missing in the above two lines?

Copy link
Member Author

@mgage mgage Dec 31, 2017

Choose a reason for hiding this comment

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

Good catch. I'd also missed a =cut which would have wiped out the definition of the L and R
decomposition functions. Here is the whole segment in context:


=head3 Accessor functions

(these are deprecated for direct use.  Use the covering Methods
 provided by MathObject Matrices instead.)
 
L($matrix) - return matrix L of the LR decomposition
R($matrix) - return matrix R of the LR decomposition
PL($matrix) - return permutation matrix
PR($matrix) - return permutation matrix
Original matrix is  PL * L * R *PR = M 

Obtain the Left Right matrices of the decomposition
and the two pivot permutation matrices
the original is M = PLLR*PR

=cut

kleene
normalize
solve_LR (also solve())
order_LR (also order()
Copy link
Member

Choose a reason for hiding this comment

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

Missing a close paren. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I fixed these and added some clarifying remarks. For a more complete description one still needs to consult the MatrixReal1.pm documentation.

=item Change the value of a matrix entry: C<change_matrix_entry($A,[2,3],50);> changes the [2,3] entry to the value 50.
=item Change the value of a matrix entry: C<change_matrix_entry($A,[2,3],50);>

changes the [2,3] entry to the value 50.

=item Construct an n x n identity matrix: C<$E = identity_matrix(5);>
Copy link
Member

@dpvc dpvc Dec 30, 2017

Choose a reason for hiding this comment

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

Note: there is already a MathObject method for that: Value::Matrix->I(5).

Copy link
Member Author

@mgage mgage Dec 31, 2017

Choose a reason for hiding this comment

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

True, but I believe Value::Matrix->I(5) was added after MatrixReduce.pl was created. In any case MatrixReduce.pl identity_matrix is simply an alias for Value::Matrix->I(5) and is part of a family of functions creating elementary matrices.


sub convert_to_array_ref {
my $input = shift;
if (ref($input) eq 'Value::Matrix' ) {
Copy link
Member

Choose a reason for hiding this comment

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

This won't catch subclasses of Value::Matrix, so perhaps if (Value::classMatch($input,"Matrix") { would be better (this will return false if $input is not a MathObject at all, so should be safe).

Copy link
Member Author

Choose a reason for hiding this comment

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

Accepted change. Thanks.

@dpvc
Copy link
Member

dpvc commented Dec 30, 2017

I made a few comments. Looks good otherwise. I haven't actually run anything, though. If you need me to do that level of testing, let me know, and I'll see if I can work on it next week.

@mgage
Copy link
Member Author

mgage commented Dec 31, 2017

I don't think you need to do extensive testing -- other than making sure it compiles. I'm doing this to try to catalog all the matrix related commands that currently exist -- writing up at least partial documentation along the way. The next step will be to consolidate and refactor the overlapping commands so that they are easier to maintain and to use -- that step will require additional testing. Meantime I think the upgraded documentation can be pulled into develop so I don't forget what has been done so far. -:)

@selinger
Copy link
Contributor

selinger commented Dec 31, 2017 via email

# Conflicts:
#	macros/MatrixReduce.pl

fixed.
@mgage
Copy link
Member Author

mgage commented Jun 3, 2018

Cleaned up the conflict with MatrixReduce.pl

@mgage mgage merged commit e935f2b into openwebwork:develop Jun 3, 2018
@mgage mgage deleted the develop_328 branch June 3, 2018 16:15
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.

3 participants