GeoStat-Framework / pentapy

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

[Very close to Bug] Tests are not exhaustive, scalable, and could have meaningless results #25

Open MothNik opened 5 months ago

MothNik commented 5 months ago

The coverage shows 80%, but only because some lines were hit during the unit tests that only consider one error case and one normal case with n=1_000. I would consider this a bug even though it works because this basically means that the package is mostly untested despite the coverage. Edge cases are not covered which lead to #24 . On top of that, the most important functionality - the input and error checks - had zero coverage which lead to #23 . It might seem tedious and a waste of time to test those, but cutting short here can easily backfire.

Besides, the test if the x for solving Ax = b is correct by looking at the residuals is very risky. A is initialised randomly, but random pentadiagonal matrices are not guaranteed to be well-conditioned. Looking at the residuals can thus be completely meaingless because the solver might work correctly, but just run into numerical issues because A doesn't want to be solved. I've written a dedicated function to generate a well-conditioned pentadiagonal A and as one can see, a lot of logic is required to guarantee good condition numbers. With this, the solution x can be checked, but still not via the residuals, but against the solution x_ref obtained by a well-tested standard like the LAPACK Partially Pivoted Banded LU solver wrapped in scipy.linalg.solve_banded.

I fixed all of this in one go with #11 by completely dropping unittest in favour of pure pytest and leveraging parametrized tests that test all relevant combinations with the same test function (including edge cases and all different sorts of input handling). To reduce the load when running the tests, I added pytest-xdist for parallelized tests. Required utility functions are doctested during the pytest runs to ensure that they are also operating correctly. With this, many different sizes and matrix layout (full, banded row, banded column) can be tested with just a single test function

MuellerSeb commented 3 months ago

Shame on me. Since this package was just a side-project, I didn't put too much work into it. Thanks for your work!