-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Convert decnum to binary64 (double) instead of decimal64 #2949
Conversation
@leonid-s-usov something like this? |
559ff54
to
af9551e
Compare
src/jv.c
Outdated
ctx->digits = DEC_NUBMER_DOUBLE_PRECISION; | ||
ctx->emax = 1023; | ||
ctx->emin = -1022; | ||
ctx->round = DEC_ROUND_HALF_EVEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
src/jv.c
Outdated
ctx->emin = -1022; | ||
ctx->round = DEC_ROUND_HALF_EVEN; | ||
ctx->traps = 0; | ||
ctx->clamp = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this i think is about IEEE 754 clamping which wikipedia describes as:
Clamped: a result's exponent is too large for the destination format. By default, trailing zeros will be added to the coefficient to reduce the exponent to the largest usable value. If this is not possible (because this would cause the number of digits needed to be more than the destination format) then an overflow exception occurs.
Not sure what disabling would mean. Our current setup with decimal64 enables it also.
src/jv.c
Outdated
ctx->emax = 1023; | ||
ctx->emin = -1022; | ||
ctx->round = DEC_ROUND_HALF_EVEN; | ||
ctx->traps = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i understand the decnum code traps are used to SIGFPE on some error conditions, probably not what we want and not what we do for dec_ctx_key above.
I have spent some time today diving deeper into the topic. I discovered an unfortunate confusion I had about the min and max exponents which I might have passed on with my former comments. This is so unexpectedly complicated! When binary64 format is talking about an exponent range of -1022..1023, it implies base 2 - hence the "binary" in the name. However, the decContext is always about decimal exponents, which explains why the numbers are -383 and 384 for decimal64. Another thing I've been researching is the included
It's also obvious from the code that conversion to a decDouble is done by calling to a decimal64.
I'm still confused as to how decDouble will differ, but apparently, it defines 1 more digit in string representation compared to decimal64:
I think that more research is needed. Specifically, it seems like our prior approach was close. |
What if instead of changing the definitions we just use the
|
It could be that our use case doesn't exactly fit a pure decimal64 application, where the life cycle is to consume string number representation, work with a decimal64 format, and then render it back into a string or serialize into a container format to later unpack as decimal64 again. We are trying to make use of the decimal number "invisible" from the perspective of someone who expects JQ to utilize IEEE double precision binary floating point representation, and it's clearly stated that to preserve the binary64 through a conversion to decimal and back one would need 17 digits of decimal precision.
So, maybe what we'd like to do is initialize the context as DECIMAL64, but then increase the precision to 17 digits. |
I'm convinced now that the proper way to look at the task at hand is to render our number into a decimal string that can be fed into Stated this way, we can even argue that the "reduce" step is optional - we can just take the result of the From this perspective, I think that the right course of action would be to call the decNumberReduce function with a local on-stack context that is initialized with |
Make sense. I somehow overlooked that even when there was a lack of base configuration.
Ideally if possible I think we want something that behaves as most major javascript imeplementations when doing integer operations. That would cause least confusion and seems to be what most users expect.
Can a decimal64 represent all exact integers that a binary64 can represent? or the modified decimal64 to 17 digits is exactly that?
Ok, so if I understand correctly we could remove |
af9551e
to
b8249e6
Compare
@leonid-s-usov pushed a variant that i hope is close to what you suggested |
This is what the JSON spec suggests and will also be less confusing compared to other jq implementations and langauges. Related to jqlang#2939
b8249e6
to
f3610e7
Compare
As I understood, while decimal64 and binary64 are very close in their capabilities, they differ both in precision and dynamic range: decimal has a wider exponent range but a little worse precision. When one wants to convert binary64 to decimal and back, they won't arrive at the same binary64 unless they use at least 17 decimal places of precision, and obviously a large enough exponent range. While decimal64 has more than enough exponent range, its precision of 16 places is not enough for the task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope we got it right this time 😅
Hope so too, lots of fiddling with this 😅 Are there some more tests we should add? current we test the bondries of the "exact integer" range and the random big (?) number from before. |
Reminder: does some documentation or wiki pages needs update? ex: https://github.com/jqlang/jq/wiki/FAQ#numbers |
I thought maybe we could compare the results with a node script output, like you did in the other ticket, but otherwise the boundaries tests you've added seem sufficient. I don't quite remember if our testing system allows for arbitrary shell commands.
I don't think so, I believe we've just fixed a regression introduced by our earlier fix |
There is https://github.com/jqlang/jq/blob/master/tests/shtest if we install node. But i'm thinking maybe we can relay on this not changing anytime soon for node? :) then some jq.test tests should be enough.
Ok! |
Thank you! =) |
This is what the JSON spec suggests and will also be less confusing compared to other jq implementations and langauges.
Related to #2939