search

GeoStat-Framework / pentapy

A Python toolbox for pentadiagonal linear systems
https://pentapy.readthedocs.io/
MIT License
14 stars 4 forks source link

Use np.asarray to prevent copies where not needed #17

Closed derb12 closed 3 years ago

derb12 commented 3 years ago

Follow-up to issue #16. This pull request switches to using np.asarray instead of np.array in pentapy.core.solve wherever shift_banded is not used on the input in order to avoid creating unneeded copies of array inputs.

derb12 commented 3 years ago

I didn't add tests yet since I wanted to get your feedback on how you wanted them done. For testing, I would just make a copy of the input matrices (and rhs if you want to be thorough) before calling solve and assert np.array_equal on the input and the copy to ensure the input was not modified within the solve function. This ensures I didn't call np.asarray in a branch where shift_banded is used. I can either:

(1) Add the tests to the three solve calls in the existing test_solve1 and test_solve2 tests and leave the other solvers untested. (2) Do (1) plus create optional tests for solvers 3, 4, and 5 that would be skipped if scipy or scikit-umfpack are not installed. (3) Make completely separate tests that only test this change for solvers 1, 2, and optionally 3-5.

Since this pull request is such a simple change, I'd be inclined to do (1), but I have no problem doing the others if you want to be more thorough and allow testing the other solvers if the optional dependencies are installed.

MuellerSeb commented 3 years ago

Hey @derb12, thanks for your sharp eye. If you are really willing to add tests, I would be totally fine with option 1 but personally I wouldn't have added tests for this marginal change and I am also fine with just merging this PR as is. You decide :wink:

derb12 commented 3 years ago

I'm also fine with not adding tests haha. Thanks.