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

default test tolerances assume Float64 precision and numbers ~ 1 #269

Open
stevengj opened this issue Jan 31, 2023 · 1 comment
Open

default test tolerances assume Float64 precision and numbers ~ 1 #269

stevengj opened this issue Jan 31, 2023 · 1 comment
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@stevengj
Copy link

The default test tolerances here seem wrong:

function test_scalar(f, z; rtol=1e-9, atol=1e-9, fdm=_fdm, fkwargs=NamedTuple(), check_inferred=true, kwargs...)

  1. The default rtol = 1e-9 assumes double precision. Better to follow isapprox here and use sqrt(eps(float(T)))?
  2. By setting a nonzero default atol you are implicitly assuming quantities of order unity. This corresponds to an absurdly large tolerance if the functions happen to be scaled to have very small magnitude. Better to follow isapprox here and default to atol = 0.
@oxinabox oxinabox added good first issue Good for newcomers bug Something isn't working labels Mar 8, 2023
@oxinabox
Copy link
Member

oxinabox commented Mar 8, 2023

This makes sense.
It should be a negligible change, and we can use the type of the input to determine the type we should use for rtol, at least for the test_scalar case. Might be a little more fiddly for the test_rule and test_frule.

We should check if this breaks anything downstream, but assuming it doesn't happy enough to see this in a bug fix release.
Otherwise we might have to do more thinking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants