BUG: test, fix loading structured dtypes with padding - #12358

also closes #8100

now will round trip fields with name == '' if the field is not a void type

I looked at doing this a while ago, but I got sidetracked by the issue that this strategy will "lose" the aligned flag:

>>> arr = np.array([(1, 2), (3, 4)], dtype=np.dtype([('a','u2'), ('b','u4')], align=True))
>>> np.save('test', arr)
>>> np.load('test.npy').dtype
dtype({'names':['a','b'], 'formats':['<u2','<u4'], 'offsets':[0,4], 'itemsize':8})
>>> arr.dtype
dtype({'names':['a','b'], 'formats':['<u2','<u4'], 'offsets':[0,4], 'itemsize':8}, align=True)

Then I started working on trying to avoid that, by making the aligned flag be recomputed on the fly when accessed (there is no fundamental reason it needs to be stored as a flag - we can always re-compute if something is aligned). That led to snowballing problems I never finished.

FWIW my implementation of descr_without_padding (which I pasted in another PR) was very similar to yours. Here it is:

def descr_to_dtype(descr):
    if isinstance(descr, str):
        # descr was produced by dtype.str, so this always works
        return numpy.dtype(descr)

    fields = []
    offset = 0
    for field in descr:
        if len(field) == 2:
            name, descr_str = field
            dt = descr_to_dtype(descr_str)
        else:
            name, descr_str, shape = field
            dt = numpy.dtype((descr_to_dtype(descr_str), shape))

        # ignore padding bytes, which will be void bytes with '' as name
        # (once blank fieldnames are deprecated, only "if name == ''" needed)
        is_pad = (name == '' and dt.type is numpy.void and dt.names is None)
        if not is_pad:
            fields.append((name, dt, offset))

        offset += dt.itemsize

    names, formats, offsets = zip(*fields)
    return numpy.dtype({'names': names, 'formats': formats,
                        'offsets': offsets, 'itemsize': offset})

I would also recommend that we simultaneously deprecate blank fieldnames in this PR, because the code here will be unable to deal with them. I did it by adding this to _convert_from_dict around line 1169:

        if (PyObject_IsTrue(name) == 0) {
            if (DEPRECATE("Blank field names are deprecated and will result in "
                          "an error in the future.") < 0) {
                Py_DECREF(tup);
                goto fail;
            }
        }

@ahaldane your function does seem to be simpler. It does handle empty names as long as they are not a void type, but I have added the deprecation since it is confusing to have blank fields on purpose.

As for alignement=True I think we should punt on that for now, as we have never been able to round-trip the attribute so we are not breaking backward compatibility here. Calculating it on the fly would force non-aligned dtypes that happen to specify an aligned dtype using offsets to be aligned after round-tripping. I think it must be part of the storage protocol. One solution would be to use str(dtype) rather than dtype.descr but that should be a separate PR.

rebased and squashed to two commits since there were merge conflicts, so I guess that also implies restarting review

I'd rather leave the deprecation to another PR, just so that it can actually be discussed in isolation.

Besides style comment above, LGTM. This was less painful that I thought. Thanks @mattip!

Thanks Matti.

I'm wondering why np.load() was special-cased here? Isn't it a more general problem?

I.e., looks like #7797 is still broken? (it would need a very similar fix)

np.save does not use PEP3118 nomenclature for the dtype format.

Ah indeed, my bad - apologies.

Fixing #7797 would require fixing/rewriting _dtype_from_pep3118() which I'm not sure anyone wants to touch.