-
Notifications
You must be signed in to change notification settings - Fork 298
Add GRIB2 workaround helper for int32->uint32. #1481
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
Conversation
ae31dfc to
b3d1c36
Compare
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 would not seem to be that accurate a name for this particular test! Perhaps test_non_zero?
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.
Copy'n'paste error!
b3d1c36 to
a90c631
Compare
|
Looks good to me 👍 |
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.
My understanding is that in sign and magnitude 0x80000000 is the value with the sign bit set i.e. 0b10000000000000000000000000000000, which corresponds to -0. As we set lower bits the result is getting increasingly negative. In two's complement this number is the most negative number we can represent (i.e. -2147483648) and as we set other bits we get less negative. In python 0x8000000 as a literal is 2147483648. If we have a signed int32 coming in (between -2147483648 and 2147483647) and for negative numbers we subtract from 2147483648 we shift from [-2147483647 to 2147483647] to [2 * 2147483648 to 2147483649 and 0 to 2147483647]. In hex this looks like [0x100000000, 0xFFFFFFFF, 0xFFFFFFFE, ..., 0x80000001 to 0x00000000, 0x00000001, ..., 0x7FFFFFFF]. So if we assume we can ignore -2147483648 (ie. limit range to 32 bit sign and mag) we get out [0xFFFFFFFF, ..., 0x80000001, 0x00000000, ..., 0x7FFFFFFF], which is [-2147483647, ..., -1, +0, ..., 2147483647] when interpreted as sign and mag. 👍
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 only make sense if the incoming value lies in the range -2147483647 to 2147483647. Feel free to correct me. If this is indeed the case, I think we should consider a check on the incoming value.
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 only make sense if the incoming value lies in the range -2147483647 to 2147483647.
True.
I think we should consider a check on the incoming value.
I did consider that originally, so I'm happy to add it.
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.
Do we need to consider endianness?
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.
Good point. I think so! 😱
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.
Or maybe not ... hmmm ... I'll have a think.
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've plumped for, "no".
GRIB2 is always big-endian. We can assume the ECMWF GRIB API takes care of this when operating on a little-endian platform - i.e. the Python int returned by grib_get should have the same logical value on all platforms. We can also assume grib_set takes care of byte-order when mapping from the logical value of a Python int to the on-disk bytes. Since the transformation in this fixup function is operating on the logical values (not any particular bit pattern) it is independent of the native byte order. QED. [If there were a crossed-fingers emoticon I'd used it here!]
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 think we're ok. That might be because it's getting late and I've already merged the other PR.
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.
🎉 consensus.
|
Give me shout tomorrow when you've added the range check and I'll merge. |
Add GRIB2 workaround helper for int32->uint32.
Partner in crime to #1480.