search

markrogoyski / math-php

Powerful modern math library for PHP: Features descriptive statistics and regressions; Continuous and discrete probability distributions; Linear algebra with matrices and vectors, Numerical analysis; special mathematical functions; Algebra
MIT License
2.33k stars 240 forks source link

Add row/columnAddVector to NumericMatrix #462

Closed Aweptimum closed 1 year ago

Aweptimum commented 1 year ago

Adds the feature discussed in #459

There are two decisions I made that you might want to consider before merging

  1. At first, I decided to match the behavior of row/columnAdd where 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 explicit k passed into row/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 return self to avoid adding imprecision and creating a new matrix.
  2. In the tests, I used MatrixFactory::createNumeric to 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.

markrogoyski commented 1 year ago

Hi @Aweptimum,

Thank you for your pull request!

Before I can run the CI tests, please either merge the latest develop branch into your branch and push the changes to the pull request, or rebase based on develop and push the changes. There are some updates in develop since your PR that will affect the success of the unit tests.

Thanks! Mark

Aweptimum commented 1 year ago

Done! I had noticed the same precision failures as in the other open PR, nice work :)

coveralls commented 1 year ago

Coverage Status

Coverage: 99.962% (+0.03%) from 99.934% when pulling 21b83d5c08b4036b0bde053e2f45c788676a48f4 on Aweptimum:addVector-ops into 7275502af59c016b6ff5f8c8491ef54b8f87004b on markrogoyski:develop.

Aweptimum commented 1 year ago

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 \r\n. I have VSCode set to use \n instead of \r\n and haven't touched any other files, so I'm not sure what's going on.

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?

markrogoyski commented 1 year ago

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, Mark

Aweptimum commented 1 year ago

Cool, thanks Mark! I had no other changes planned for this branch.

markrogoyski commented 1 year ago

Thanks again for the PR. This functionality will go out in the next release, v2.8.0.

Aweptimum commented 1 year ago

You're welcome, thanks for fiddling with it!

Aweptimum commented 1 year ago

I realized the next day I forgot to add it to the docs 🤦‍♂️ should probably just make that a part of #461

markrogoyski commented 1 year ago

I added them to the README in the develop branch. No worries. Thanks.