Closed kellertuer closed 1 year ago
I agree with your ideas, having the warn
functionality in is_something
functions could be useful. Also isapprox
with error/warning options would be nice.
And yes, we can freely extend functions that take the manifold with additional arguments.
Do you have a good idea what to put into the last positional argument to distinguish them? That was easy for error-throwing yes/no ;)
It can be just a symbol as in your example, that would be the easiest choice.
Ok, and then just with an if I suppose since it is really just:
if error_type == :error
...
else if error_type == :info
...
else
... # default true/false
end
ok that sounds good. For the is_point
things I have to check a little when these cases fall back to existing ones. But since this is all layer 1 it should be doable quite generically I think.
Yes, something like that, except error_type === :error
is better than error_type == :error
(easier for the compiler to optimize).
ah yes, sure. Will do that next here, then (after my current PR).
Since I am currently restructuring our tests, I noticed two things:
is_point
provides information why is returns false (if activated), but one could maybe even make it non failing?My idea would be something like
is_point(M, p true)
stays as is to be non-breaking but is the same asis_point(M, p, :error)
throws an error ifp
is not a point onM
is_point(M, p, :warn)
displays a warning – the test would fail but the test set would continueSure symbols are not the best choice here, let me know if you have a better idea how to distinguish both cases
isapprox(M,p,q)
andis approx(M,p,X,Y)
are quite hard to read when they fail, so could we maybe extend those functions to act similar tois_point
that is an error message can be activated that checks first whether it was atol or rtol which made the test fail and (similar to above) either errors or warns? This could be a last positional argument as well for both, I think.I also started a discussion at https://discourse.julialang.org/t/isapprox-in-tests-and-a-more-precise-result-reason-for-failure/89648 – but then remembered, that we are defining our own
isapprox
es anyways, so with theM
upfront we can add the new positional argument without “committing type piracy”.Let me know what you think.