On Thu, Aug 24, 2023 at 4:58 PM Andrew Knyazev <andrew.knyazev@ucdenver.edu> wrote:
Ralf, thanks for your quick response! I agree with you in principle, of course. However, the specific benchmark I am editing https://github.com/scipy/scipy/pull/18954/ (search ASV for "sakurai" and "mikota") has its peculiarities that make the standard approach impractical: - One of the solvers under the benchmark comparison, eigsh, has an internal check for convergence and fails if no convergence. The other, lobpcg, only gives a warning if the accuracy is not reached but is designed to never give an error. The third solver is dense so supposedly just never fails. An external assert is needed to make these solvers comparable in a benchmark.
An assert will never *make* the solvers comparable. Correctly configuring each of the solvers to do the equivalent task makes them comparable.
- Some of the benchmark tests take long time to run because the matrix size needs to be large enough to make the benchmark meaningful. These runs are simply too long for the test suite, so their accuracy cannot be tested there.
We can mark tests as slow or even extra-slow. If we *need* a slow test to ensure that the solvers are correct, then this is the way to do it. We do not run the benchmarks to find errors in the library. https://numpy.org/devdocs/reference/testing.html#labeling-tests https://docs.scipy.org/doc/scipy/dev/contributor/devpy_test.html#tips - The actual benchmarks in this PR are currently tuned such that there are
no actual failures with the current versions of the solvers benchmarked. Practically speaking, we have a rather philosophical discussion like "to be or not to be" on whether a single line assert that currently has no effect can be present in this benchmark in this specific PR or should not be allowed as a matter of principle.
We need to be clear on what it's for. If, as you've stated, it's to ensure that the *solvers* are correct, then no, it doesn't belong in the benchmark. That's a deficiency in the test suite and appropriate tests need to be filled out there. It's not that we object to the assert in the benchmark per se, it's that we object to not fixing the hole in the test suite that you claim is there. If, on the other hand, the purpose of the assert is to ensure that the *benchmark* remains valid (i.e. that all of the individual settings to each solver are such that they are apples-to-apples), then that's fine, IMO. When the assert in the benchmark fails, where do you think you will make fixes, in `scipy/sparse/` or `benchmarks/`? If the latter, then it's fine. If it's the former, then add it to the test suite. -- Robert Kern