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

remove strict float comparisons #28

Open
danieleades opened this issue Feb 9, 2021 · 2 comments
Open

remove strict float comparisons #28

danieleades opened this issue Feb 9, 2021 · 2 comments

Comments

@danieleades
Copy link

this library uses a bunch of strict float comparisons.

would you consider a pull request to implement slightly safer float comparisons? possibly using float_cmp or simply making use of the num_traits::float::Float::epsilon method (either way will probably require a where T: num_traits::Float: trait bound)

unless there are sound reasons we can assume they are identically equal in these comparisons?

if so, would you consider a pull request to silence this lint? Strict float comparisons are denied by Clippy, and will prevent using Clippy in any CI pipeline.

@That3Percent
Copy link
Owner

I would accept a PR to silence this lint, but not to remove strict equality comparisons. Thanks for asking!

Since this is a serialization library, one of the key tenets is that if you round trip data through it, the binary representation of that data should be unchanged. We don't want to be making assumptions for our users about what is a tolerable loss of data because we do not know their use-case a-priori.

So, while float_cmp has it's place, I don't believe that place is here. It's more for doing math like checking if a point is inside a triangle and similar.

There is an exception to this binary equivalence rule when lossy encodings are enabled - but that is opt-in, has a configurable tolerance, and IIRC is not the part of the code where these equality checks are occurring. I might accept a PR that used float_cmp when that feature is enabled The lint silencing would still be required since there would now just be multiple code paths.

@danieleades
Copy link
Author

ah thanks for the explanation. I had a feeling it was going to be something along those lines. I'll submit a pull request to supress the lint and call it a day.

I will also take a look at the lossy encodings and see if it's worth touching the float comparisons there

@danieleades danieleades mentioned this issue Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants