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

Accuracy with Float32s is bad #98

Open
willtebbutt opened this issue Jul 11, 2020 · 6 comments
Open

Accuracy with Float32s is bad #98

willtebbutt opened this issue Jul 11, 2020 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@willtebbutt
Copy link
Member

@wesselb and I had a discussion about this a while ago, and I completely forgot to raise an issue about it. While FiniteDifferences accuracy for functions of Float64s is as you would expect, other types are a different matter. Of particular concern is Float32 as it's almost certainty the next most used type.

IIRC the issue is an issue of defaults. Specifically this one. float always yields a Float64, and eps(::Float64) is very different from eps(Float32). This changes the step-size calculation here.

If someone has time to look into this it would be greatly appreciated. I'm not entirely sure what the correct solution is, but a good approach to solving it might be

  1. recap how in principle to set the eps parameter that is causing the problems (perhaps @wesselb could help here?)
  2. figure out how to set eps based on this and make the appropriate changes
  3. test that it works
@willtebbutt willtebbutt added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed labels Jul 11, 2020
@wesselb
Copy link
Member

wesselb commented Jul 13, 2020

If I'm not mistaken, the parameter eps is used to calculate the round-off error and should be set to the machine epsilon of the corresponding data type. The issue is that TINY is Float64, so bound always is Float64, even if the function outputs Float32s. In that case, eps is incorrectly set to the machine epsilon of Float64s, so the step size will be off.

For the particular case of Float64s and Float32s, most likely changing TINY to 1f-20 will fix most of the issues.

@willtebbutt
Copy link
Member Author

Might make sense to cast it explicitly wherever it's used to the element type of the thing that it's being added to. This way if (for some reason) we ever use Float16s or some extended-precision stuff we're protected.

@wesselb
Copy link
Member

wesselb commented Jul 13, 2020

Yes, that's a good suggestion. We should carefully check the code, to ensure that the assumption of Float64s is not also subtly baked in in other places.

@willtebbutt
Copy link
Member Author

willtebbutt commented Jul 13, 2020

Actually, it might make sense just to replace TINY with a function called add_tiny or something to prevent accidentally forgetting to cast stuff.

@wesselb
Copy link
Member

wesselb commented Jul 13, 2020

Yes, that's an even better solution.

@wesselb
Copy link
Member

wesselb commented Jan 11, 2021

Do we have a good example where the accuracy for Float32s is not what we would like it to be? I wonder if v0.12 resolves these issues.

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 help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants