JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.49k stars 5.46k forks source link

more informative @test x ≈ y failure message #55812

Open stevengj opened 1 week ago

stevengj commented 1 week ago

It would be nice to have a more informative failure message for a @test x ≈ y that:

  1. If one of the operands iszero, and the caller didn't pass a explicit atol, warn them that they might want to specify an atol (or rewrite e.g. x - y ≈ 0 to x ≈ y), and direct them to the isapprox docs.
  2. If it failed a relative tolerance (rtol) check, print out the computed relative error norm(x - y) / max(norm(x), norm(y)). See also #55613

(I feel like this has been raised before, but I can't find an issue?)

barucden commented 1 week ago

I think the first point here is a new one, otherwise: #55613

Anyway, how should one implement it without duplicating the logic of isapprox? Or how to avoid making ~Pkg~Test depend on LinearAlgebra for norm of vectors?

stevengj commented 1 week ago

I think you have to duplicate some of the logic. Fortunately that logic is very simple.

Yes, that would make Test (not Pkg) explicitly depend on LinearAlgebra, but I don't see that as a big deal. LinearAlgebra is already implicitly loaded inside Julia, and is used when you call isapprox whether or not you explicitly depend on it.

(Or we could add a Base.isapprox_showerror(io, x, y; kws...) function, and then add additional methods in LinearAlgebra for arrays, then call this from Test. This would have the advantage of being easier to extend for more types. It would still involve some duplicated logic, though.)

nsajko commented 1 week ago

LinearAlgebra is already implicitly loaded inside Julia

But that situation isn't ideal: #51633

martinholters commented 3 days ago

Ideally, the special printing for isapprox (at least when the arguments are linear algebra objects) would be placed in a package extension that gets loaded when LinearAlgebra is loaded in addition to Test. No idea how much work it would be to add a plugin-like system for printing to Test to make this feasible.

stevengj commented 3 days ago

@martinholters, that doesn't make sense here. You don't have to load LinearAlgebra explicitly — it is already loaded implicitly by base, and is already used for array ≈ array whether or not the LinearAlgebra package is loaded by the user.

(Even if some portions of LinearAlgebra are removed from the sysimg in the future, it seems like norm will have to remain.)

martinholters commented 2 days ago

Hypothetically, if one is not interested in anything related to linear algebra, one could maybe excise LinearAlgebra, but still would like use Test. And Test then depending on LinearAlgebra, even though one doesn't want to do any isapprox test, would be a bit unfortunate. But granted, that is so hypothetical that it should not keep us from improvements to test output now.

stevengj commented 2 days ago

Yes, things like isapprox(vector, vector) and inv(matrix) are in Base now, and cannot be removed in Julia 1.x due to backwards compatibility. So I don't think we should limit the capabilities of @test based on a hypothetical Julia 2.x many years in the future.