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

correct some POD #1188

Merged
merged 1 commit into from
Feb 13, 2025
Merged

correct some POD #1188

merged 1 commit into from
Feb 13, 2025

Conversation

Alex-Jordan
Copy link
Contributor

This is phase 1 of my attempt to replace #1076. It seemed to me that some of the POD for lib/Value/Matrix.pm is just incorrect or unclear. So this tries to fix that.

I started thinking of MathObject Matrices as n-dimensional tensors, to help with some confusion when is not a "normal" 2D matrix. I'm using "tensor" some in the POD. There are inconsistencies with how things work when you have a 1D tensor versus nD with n > 1. The inconsistencies would be resolved if 1D tensors were presented as column vectors instead of row vectors. But I suppose we can't make a change like that for backward compatibility reasons, for problems out there that use 1D matrices. With the inconsistencies kept, a lot of the POD is going to need to be like "it works this way for a 1D tensor/matrix but it works this other way for higher D".

Phase 2 will be to rearrange/clean up the POD in the manner of #1076. But that should be easier to review as it will just be moving things around with perhaps some minor rewording.

Phase 3 will be to enhance some things in the existing methods. Specifically I think that row() and column() should be allowed to take multiple index arguments to produce submatrices (e.g. the submatrix of the 1st and 3rd rows).

Phase 4 will be to add @pstaabp's new methods.

Somewhere in there, also add the tests. Maybe some with Phase 3 and some with Phase 4.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

@dpvc
Copy link
Member

dpvc commented Feb 12, 2025

The inconsistencies would be resolved if 1D tensors were presented as column vectors instead of row vectors.

Do you mean that you would like Matrix(1, 2, 3) to produce what Matrix([1], [2], [3]) currently does as opposed to what Matrix([1, 2, 3]) does? This would actually be a 2D matrix (3 x 1) rather than a what is essentially a row vector.

Or do you mean that 1D matrices should be treated more like ColumnVector(1, 2, 3) (in terms of multiplication, string and TeX output, etc.)?

Would Matrix(1, 2, 3) be different from Matrix([1, 2, 3])?

Would Matrix(1, 2, 3)->row(1) return just 1 and Matrix(1, 2, 3)->column(1) return [1, 2, 3]?

The original idea was that you passed Matrix() the rows, so Matrix([1, 2, 3]) should be what amounts to a 1 x 3 matrix, but I could see how that would suggest that Matrix(1, 2, 3) should be a 3 x 1 matrix.

@Alex-Jordan
Copy link
Contributor Author

First of all, I think that things are the way they are right now, and making changes about this is probably off the table. Because problems out there in the wild handle 1D matrices the way they handle them now, and a change like I am thinking would disrupt how they work, possibly even breaking them.

But to answer your question, I was thinking this one:

Or do you mean that 1D matrices should be treated more like ColumnVector(1, 2, 3) (in terms of multiplication, string and TeX output, etc.)?

Right now, if I make a 1D matrixMatrix(1,2,3) and display it, I see:

[1  2  3]

If I want to get to an entry, I specify an index, and I walk horizontally to get that index.

And if I make a 2D matrix Matrix([1,2,3], [4,5,6]) and display it, I see:

[1 2 3]
[4 5 6]

If I want to get to an entry, I specify two indices, and I walk vertically then horizontally.

And if I make a 3D matrix Matrix([[1,2],[3,4],[5,6]], [[7,8],[9,10],[11,12]]) and display it, I see:

[[1 2] [3  4] [ 5  6]]
[[7 8] [0 10] [11 12]]

If I want to get to an entry, I specify three indices, and I walk vertically, then horizontally, then horizontally again. The "6" is in the (1,2,2) position.

And if I make a 4D matrix, Matrix([[[1,2],[3,4]],[[5,6],[7,8]]], [[[9,A],[B,C]],[[D,E],[F,0]]]) and display it, I see:

[[1 2]   [5 6]]
[[3 4]   [7 8]]
[             ]
[[9 A]   [D E]]
[[B C]   [F 0]]

If I want to get to an entry, I specify four indices, and I walk vertically, horizontally, vertically, horizontally. The "6" here is in the (1,2,1,2) position.

So as we go up in dimensions, how you walk to an entry has a sort of inconsistent pattern:

H
VH
VHH
VHVH
VHVHH
VHVHVH
...

And if 1D matrices were handled as column vectors, then the index walk pattern would instead be:

V
VH
VHV
VHVH
VHVHV
VHVHVH
...

Of course with that change, to make something that looks like a row vector would need Matrix([1], [2], [3]). But that is the complication you currently need for something that looks like a column vector.

The original idea was that you passed Matrix() the rows, so Matrix([1, 2, 3]) should be what amounts to a 1 x 3 matrix, but I could see how that would suggest that Matrix(1, 2, 3) should be a 3 x 1 matrix.

I think this characterizes the inconsistency too. If you pass multiple arguments to Matrix, one thing happens if they are array references (they are treated as rows) and something else happens if they are numbers (they are treated as individual entries for one row).

Would Matrix(1, 2, 3) be different from Matrix([1, 2, 3])?

Yes, under the change the first one is a 1D matrix that looks like a column vector when displayed. And is upgraded to a 2D 3x1 matrix as needed for multiplication with 2D matrices. Whereas the second one would be a 2D 1x3 matrix from the start.

Would Matrix(1, 2, 3)->row(1) return just 1 and Matrix(1, 2, 3)->column(1) return [1, 2, 3]?

Matrix(1, 2, 3)->row(1) would return Matrix(1). Matrix(1, 2, 3)->column(1) would return Matrix(1, 2, 3).

(Note that presently, Matrix(1, 2, 3)->row(1) returns Matrix(1, 2, 3), but Matrix(1, 2, 3)->column(1) doesn't return a Matrix at all. It returns Real(1).)

@Alex-Jordan
Copy link
Contributor Author

Sorry, I messed up at least one thing typing that and not being careful.

Of course with that change, to make something that looks like a row vector would need Matrix([1], [2], [3]). But that is the complication you currently need for something that looks like a column vector.

Should have saud:

Of course with that change, to make something that looks like a row vector would need Matrix([1, 2, 3]) instead of the simple Matrix(1, 2, 3). But right now to get something that looks like a column vector you need the complicated Matrix([1], [2], [3]) and while that would still work, you could instead just do Matrix(1, 2, 3).

@Alex-Jordan
Copy link
Contributor Author

This has two approvals. If it can be merged, I will proceed with phase 2. That's just rearranging the POD to be like @pstaabp's in #1076.

I'll pause after that for discussion. But I would like to review the existing methods and possibly enhance them. For example $M->row([2,3]) or $M->row(2,3) could return the submatrix of $M built from the 2nd and 3rd rows.

Also if @dpvc doesn't think my idea about 1D matrices would be a disaster for existing exercises, maybe I would do that as well.

And only after existing methods have all been reviewed/revised, I'd return to the new methods that @pstaabp is adding in #1076.

@drgrice1
Copy link
Member

I will go ahead and merge this, so you can proceed with the next phase.

@drgrice1 drgrice1 merged commit 83cc66d into openwebwork:develop Feb 13, 2025
3 checks passed
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