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

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

Open
MothNik opened this issue Jun 8, 2024 · 1 comment

Comments

@MothNik
Copy link

MothNik commented Jun 8, 2024

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

@MothNik MothNik changed the title [Bug] Tests are not exhaustive, scalable, and could have meaningless results [Very close to Bug] Tests are not exhaustive, scalable, and could have meaningless results Jun 8, 2024
@MuellerSeb
Copy link
Member

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

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

No branches or pull requests

2 participants