Add half-float (16-bit) support to struct module

[EliStevens]

BPO	11734
Nosy	@rhettinger, @mdickinson, @abalkin, @pitrou, @asvetlov
Files	cpython-struct-float16-v4.patch: Unifies implementation for non IEEE platforms, adds docs and tests. cpython-struct-float16-v5.patch cpython-struct-float16-v6.patch cpython-struct-float16-v7.patch cpython-struct-float16-v8.patch cpython-struct-float16-v9.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/mdickinson'
closed_at = <Date 2016-09-03.16:23:35.942>
created_at = <Date 2011-04-01.01:00:15.673>
labels = ['type-feature', 'library']
title = 'Add half-float (16-bit) support to struct module'
updated_at = <Date 2016-09-03.16:23:35.942>
user = 'https://bugs.python.org/EliStevens'

bugs.python.org fields:

activity = <Date 2016-09-03.16:23:35.942>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2016-09-03.16:23:35.942>
closer = 'mark.dickinson'
components = ['Library (Lib)']
creation = <Date 2011-04-01.01:00:15.673>
creator = 'Eli.Stevens'
dependencies = []
files = ['21540', '21544', '30145', '38043', '44281', '44282']
hgrepos = []
issue_num = 11734
keywords = ['patch']
message_count = 40.0
messages = ['132722', '132726', '132732', '132761', '132763', '132764', '132765', '132942', '132961', '132976', '132980', '133004', '133005', '133013', '133020', '133024', '133025', '133060', '133073', '133078', '157476', '157484', '157488', '157490', '157491', '187590', '187592', '187615', '187656', '187658', '187661', '188504', '235555', '273901', '273920', '273933', '273950', '273952', '273965', '274308']
nosy_count = 9.0
nosy_names = ['rhettinger', 'mark.dickinson', 'belopolsky', 'pitrou', 'asvetlov', 'python-dev', 'Eli.Stevens', 'mark.wiebe', 'paulehoffman']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue11734'
versions = ['Python 3.6']

[EliStevens]

Numpy 1.6.0 adds support for a half-float (16-bit) data type, but
cannot currently export a buffer interface to the data since neither PEP-3118 nor the struct module (referenced by PEP-3118) support the data type.

I am proposing that the struct module be extended to support half floats, and will be providing a patch that implements that behavior.

The current numpy implementation (1.6.0b1) uses the 'e' character to indicate half floats; I don't have a vested interest in what character is used. If consensus between CPython and numpy is reached for using a different character, I will update my patches (but I don't plan on participating in the bike shedding prior to that point).

For reference:

python-ideas thread:
http://mail.python.org/pipermail/python-ideas/2011-March/009650.html

numpy-discussion thread:
http://mail.scipy.org/pipermail/numpy-discussion/2011-March/055667.html

cython-user thread (less relevant to this issue specifically):
http://groups.google.com/group/cython-users/browse_thread/thread/6bf811409cdab89e

[EliStevens]

Initial patch; tests pass. A couple notes:

• I manually excised some commented changes to structmodule.{h,c} from the .patch; once I get a determination on if those files need to be updated or not I will discard the changes or implement what needs to be done there. Hopefully I didn't damage the patch while editing.

• The code in _PyFloat_Pack2 and _PyFloat_Unpack2 is adapted from the numpy code in https://github.com/numpy/numpy/blob/master/numpy/core/src/npymath/halffloat.c . This means that it is BSD licensed; I'm presuming that is acceptable for inclusion in CPython. If not, I'll try a clean-room implementation.

[markwiebe]

Taking a look at the patch, I see you're using the single -> half conversion routine from NumPy. This has the double rounding problem when converting double -> float -> half, so it would be better to use the double -> half routine. I implemented it with 64-bit unsigned ints, so if there are platforms not supporting 64-bit ints, something else such as using the 32-bit routine will have to be done there.

[EliStevens]

I'm thinking of taking the current float implementation and wrapping it with something like:

#if HAS_INT64_TYPE
    // double implementation goes here
#else
    // float implementation here (what's in the current patch)
    ...
#endif

Does this seem like a reasonable approach?

Is there a CPython-canonical way to spell "HAS_INT64_TYPE" and/or find out what type that is on the current platform?

[EliStevens]

I've spelled "HAS_INT64_TYPE" as follows:

+#if SIZEOF_LONG == 8 || SIZEOF_LONG_LONG == 8
+#if SIZEOF_LONG == 8
+ typedef unsigned long npy_uint64;
+#else
+#if SIZEOF_LONG_LONG == 8
+ typedef unsigned long long npy_uint64;
+#endif
+#endif
...

I left the Unpack2 function using floats, since there's no rounding issues there, and it didn't feel like the extra code complexity was justified. If there's disagreement on that point, I can make a similar change for Unpack2.

[markwiebe]

I think this won't work on Windows since there the 64-bit int is generally __int64. If you look at the long long and unsigned long long support in _struct.c you can see a better way to do this: "#ifdef HAVE_LONG_LONG" and "unsigned PY_LONG_LONG" for the type.

[EliStevens]

Ahh, yes, much cleaner. Thank you. :) I'll remove the previous version.

[mdickinson]

I've only glanced at the patch, but a couple of things:

(1) It looks as though the patch assumes that a C double is IEEE 754 binary64 format, and that a C float is IEEE 754 binary32 format. Is that correct?

If so, that's a significant break with tradition: Python's supposed to be able to work with the native doubles, no matter what their format. I, for one, wouldn't object if IEEE floating-point were made a requirement for Python >= 3.3, but this change would need to be discussed on the python-dev mailing list.

(2) The

typedef PY_LONG_LONG npy_uint64;

looks wrong to me for two reasons: first, PY_LONG_LONG is a signed type. Second, while it's guaranteed (by the C standard) that unsigned long long will have width at least 64, it's not guaranteed that it'll be exactly 64. Does the code depend on this assumption? Similarly when using unsigned int in the union for the 32-bit code; here it's not even guaranteed by the standards that unsigned int has width at least 32.

It may be better to use the PY_UINT64_T type and the HAVE_UINT64_T macros, and similarly for the 32-bit types.

[markwiebe]

The patch currently assumes IEEE 754 with byte-order matching the integer type. I see that pyconfig.h defines three possible cases when IEEE 754 doubles are supported, DOUBLE_IS_LITTLE_ENDIAN_IEEE754, DOUBLE_IS_BIG_ENDIAN_IEEE754, and DOUBLE_IS_ARM_MIXED_ENDIAN_IEEE754. The code should have some byte swapping to match it to the integer endianness.

If support for non-IEEE systems is still desired, implementing a conversion using isnan/isinf/frexp/ldexp should be pretty easy, though off hand I can't see an efficient way to extract the significand bits.

Regarding the PY_LONG_LONG, it should have been unsigned PY_LONG_LONG. Using PY_UINT64_T is better, though, since a bigger PY_LONG_LONG would cause trouble in the union.

For the UINT32, maybe just using the double/PY_UINT64_T versions is better, since there is no macro for FLOAT_IS_IEEE754? Falling back to a frexpr implementation if double isn't IEEE or there is no 64-bit integer type may be a reasonable tradeoff to support the few platforms where that's the case, and 2 instead of 3 separate conversion codes is a bit better maintenance-wise.

[EliStevens]

There seems to be some disagreement about the double-rounding issue; Mark Dickinson posted on python-ideas that he doesn't think that there is one. If possible, I think that removing code paths that aren't needed are generally a good thing, especially if I'm going to have to add support for non-IEEE floats.

I'll start working on the isnan/isinf/frexp/ldexp implementation for the cases where either we don't have a UINT64 type, or doubles aren't IEEE. Given my current schedule, I don't think that I'll have this done today; hopefully tomorrow morning. :)

[markwiebe]

There's no disagreement, since they're different cases. Taking an arbitrary double, rounding to float, then rounding to half definitely has double-rounding issues. (And I would recommend constructing an example to add to the test case to make sure you understand what's going on.) Taking two halfs, doing a primitive arithmetic operation on them as floats, then rounding back to half is what Mark was referring to on the list.

In NumPy I also tried to err more towards accuracy and performance than having each intermediate be strictly half. The einsum function retains intermediate values as floats when operating on half, for example, and that's what I recommended in the documentation.

I'd also suggest adding some more to the test suite here to verify that ties are rounding to the nearest even properly. You can see some tests for this here:

https://github.com/numpy/numpy/blob/master/numpy/core/tests/test_half.py#L124

[EliStevens]

I see the distinction about the rounding error now. Thanks for clarifying; I've added more tests as suggested.

Looking at _struct.c, line 2085:

                /* Skip float and double, could be
                   "unknown" float format */
                if (ptr->format == 'd' || ptr->format == 'f')
                    break;
                ptr->pack = native->pack;
                ptr->unpack = native->unpack;
                break;

I'm going to assume that it's okay to allow 'e' to pass through here, since the 'e' type is *always* going to be in IEEE 754 (not "unknown") floating point format, and the handling routines don't rely on the C conversions the way the float/double ones do. Is that a good assumption to make?

Also, looking at the line in the native_table declaration:

{'e',       sizeof(short),  SHORT_ALIGN,    nu_halffloat,   np_halffloat},

Does it make sense to have an entry here since there isn't actually a native representation? ATM it's implemented as just figuring out the platform endianness, and then calling the corresponding lp_/lu_ or bp_/bu_ function.

[EliStevens]

More questions:

The current _PyFloat_Pack4 doesn't round to even:

        fbits = (unsigned int)(f + 0.5); /* Round */

Is the mismatch going to be a problem? Should I change the _PyFloat_Pack4 implementation while I'm in there?