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

Adds conformance tests for floats #134

Merged
merged 7 commits into from
Nov 19, 2024
Merged

Conversation

popematt
Copy link
Contributor

Issue #, if available:

#88

Description of changes:

Adds test cases for Ion floats.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestions are all more minimal expressions of the same float value. There's no point in presenting precision that the type does not actually allow.

conformance/data_model/float.ion Outdated Show resolved Hide resolved
conformance/data_model/float.ion Outdated Show resolved Hide resolved
conformance/data_model/float.ion Outdated Show resolved Hide resolved
(then "a subnormal"
(then "f32 value"
(binary "44 00 00 00 01")
(denotes (Float "1.401298464324817E-45")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(denotes (Float "1.401298464324817E-45")))
(denotes (Float "1e-45")))

(denotes (Float "5.9604645e-8")))
(then "f32 value"
(binary "6C 01 00 00 00")
(denotes (Float "1.401298464324817E-45")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(denotes (Float "1.401298464324817E-45")))
(denotes (Float "1e-45")))

conformance/data_model/float.ion Outdated Show resolved Hide resolved
conformance/data_model/float.ion Show resolved Hide resolved
(denotes (Float "-5.9604645e-8")))
(then "f32 value"
(binary "6C 01 00 00 80")
(denotes (Float "-1.401298464324817E-45")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(denotes (Float "-1.401298464324817E-45")))
(denotes (Float "-1e-45")))

conformance/data_model/float.ion Outdated Show resolved Hide resolved
// optional '-', one or more digits, '.', zero or more digits, optional group of 'e' and one or more digits OR
// optional '-', one or more digits, 'e', one or more digits OR
// one of the non numeric float values
regex: "^-?\\d+[.]\\d*(e\\d+)?|-?\\d+e\\d+|[+-]inf|nan$"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about this earlier, but why allow the conformance tests to express floats using the same syntax as Ion decimals? Why is the e optional?

Copy link
Contributor Author

@popematt popematt Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bit of a TBD here, I guess, that didn't really get resolved. TLDR; model-float relies on the IEEE-754 string representation of floating point numbers.

We had a bit of a bootstrapping problem when it comes to testing floats. Unless we want to have model-float define values in terms of a sign, exponent, and mantissa, we need to have some alternate representation. I took the lazy/simple way and said that it should be a string that can be parsed by a floating point library (usually in the standard library of a language). Then, I ran into the problem that these libraries don't always agree on the serialization of non-numeric/finite values, so I had to specify that in a way that excluded things like +Infinity, +Inf, etc. The result is that I had to specify something for the finite float values. I could have done [0-9.e-]+, but that seemed too sloppy, so I approximated what is allowed in IEEE-754.

Anyway, although it might make it look like there's a bit of a circular definition, I don't mind changing it to Ion float syntax.

@popematt
Copy link
Contributor Author

popematt commented Nov 18, 2024

My suggestions are all more minimal expressions of the same float value. There's no point in presenting precision that the type does not actually allow.

All of the tests where you have made these suggestions are tests for subnormal numbers, so this is not actually an excess of precision.

Edit—I was wrong.

Copy link
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭

conformance/data_model/float.ion Outdated Show resolved Hide resolved
Co-authored-by: Tyler Gregg <[email protected]>
@popematt popematt merged commit 3a11107 into amazon-ion:main Nov 19, 2024
1 check passed
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 this pull request may close these issues.

3 participants