Re: should a benchmark be meaningful and assert accuracy?

On Thu, Aug 24, 2023 at 4:58 PM Andrew Knyazev <[email protected]>
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

_______________________________________________
SciPy-Dev mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://mail.python.org/mailman3/lists/scipy-dev.python.org/
Member address: [email protected]