Add row/columnAddVector to NumericMatrix #462
Conversation
|
Hi @Aweptimum, Thank you for your pull request! Before I can run the CI tests, please either merge the latest Thanks! |
Similar to columnAdd's condition that k be non-zero, but may not be necessary.
The # of elements in the vector should equal the # of rows col element
Given a column index and a vector, adds the elements of the vector to the corresponding row. Conditions are: 1. The column index must exist 2. The vector cannot be a zero-vector 3. The # of vector components must equal the # of matrix rows For 2. the zero-length check uses the error of the matrix to compare the vector length to zero.
Make sure the count of the vector components equals the number of rows in the matrix
Given a vector and a row index, adds the elements of the vector to the corresponding row Conditions are: 1. The row index must exist 2. The vector cannot be a zero-vector 3. The # of vector components must equal the # of matrix columns For 2. the zero-vector check uses the error of the matrix to compare the vector magnitude to zero.
It might be worth breaking the precedent set by row/columnAdd since it's not guaranteed a user knows they're passing a zero vector into the method. In such a case, it seems best to allow the addition rather than break.
|
Done! I had noticed the same precision failures as in the other open PR, nice work :) |
|
It seems one of my commits removed the newline in the MatrixColumnOperationsTest (probably a squash gone wrong), although putting it back locally hasn't made phpcs happy. It's failing every file in my local repo with the same error, that they all end with The RowOperations test seems to have no errors, so I can try adding the newline back to the columns test, squash it into the column length-mismatch test commit, and force push again to see if it passes the CI. You alright with that? |
|
Hi @Aweptimum, Don't worry about the newlines. With Macs/Linux/Windows sometimes it is hard to get it all right. I'll take care of the newlines. The tests are passing, which are the important check. Let me know if you have any other changes planned. Otherwise, I'll go ahead and merge it and take care of the PHPCS linting issue. Thanks, |
|
Cool, thanks Mark! I had no other changes planned for this branch. |
| if ($mᵢ < 0 || $mᵢ >= $this->m) { | ||
| throw new Exception\MatrixException('Row to add does not exist'); | ||
| } | ||
| if ($v->count() !== $this->m) { |
There was a problem hiding this comment.
FYI: This should actually be n and not m. I've fixed this and added some unit tests around it.
|
Thanks again for the PR. This functionality will go out in the next release, v2.8.0. |
|
You're welcome, thanks for fiddling with it! |
|
I realized the next day I forgot to add it to the docs 🤦♂️ should probably just make that a part of #461 |
|
I added them to the README in the |
Adds the feature discussed in #459
There are two decisions I made that you might want to consider before merging
row/columnAddwhere k is not allowed to be 0, so I added a condition to check if the given vector is a zero-vector by comparing its length to 0 (within the tolerance of the matrix). As I was writing this PR though, I realized it might best to break precedent since the user may not know the contents of the vector at run-time vs the explicitkpassed intorow/columnAdd. If you disagree, you can revert the last commit. However, I think there's little harm in letting it pass through, and it might be even better to keep the comparison and just returnselfto avoid adding imprecision and creating a new matrix.MatrixFactory::createNumericto make intellisense happy 🙃It's inconsistent with the rest of the tests, but I figured it may not matter
Let me know of any desired changes or formatting errors, I did my best to follow the guidelines.