Skip to content

BUG: construct ma.array from np.array which contains padding - #7788

Closed
jzwinck wants to merge 2 commits into
numpy:masterfrom
jzwinck:masked-with-padding
Closed

jzwinck wants to merge 2 commits into
numpy:masterfrom
jzwinck:masked-with-padding

Conversation

@jzwinck

@jzwinck jzwinck commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

A unit test included, which fails before this patch,
because dtype.descr has extra entries when padding is present.

John Zwinck added 2 commits June 29, 2016 12:52
A unit test included, which fails before this patch,
because dtype.descr has extra entries when padding is present.
@ahaldane

Copy link
Copy Markdown
Member

Nice catch!

I think this we can get this merged without too much more effort, but I have two suggestions to consider first:

First, the same type of fix may be needed a few lines down (line 426) for _recursive_set_fill_value.

Second, maybe instead of patching descr we can refactor _recursive_set_default_fill_value and _recursive_set_fill_value to avoid use of dtype.descr entirely. In my opinion none of the MaskedArray code should touch the dtype.descr attribute, since as I understand it is only meant to be used in the "PEP3118 buffer interface" and because of that converts the padding in a funny way. It would be better to only use the dtype.names and dtype.fields attributes. For example, I imagine _recursive_set_fill_value could look like

def _recursive_set_fill_value(fillvalue, dtype):
    fillvalue = np.resize(fillvalue, len(dtype.names))
    output_value = []
    for (fval, name) in zip(fillvalue, dtype.names):
        cdtype = dtype[name]
        if cdtype.names:
            output_value.append(tuple(_recursive_set_fill_value(fval, cdtype)))
        else:
            output_value.append(np.array(fval, dtype=cdtype).item())
    return tuple(output_value)

and have fill_value = np.array(_recursive_set_default_fill_value(ndtype), dtype=ndtype). (I haven't tested that code). That way we don't even need to think about the padding bytes in descr.

A similar change would be needed for _recursive_set_default_fill_value. Does that seem reasonable?

@jzwinck

jzwinck commented Jun 29, 2016

Copy link
Copy Markdown
Contributor Author

@ahaldane What you say sounds reasonable. I'm not volunteering to do it, but it did strike me that using descr there is unfortunate.

Sidebar: I was surprised to see this code was called even if I passed a fill_value argument to ma.array().

@ahaldane

ahaldane commented Jun 30, 2016 •

Copy link
Copy Markdown
Member

OK, in that case I took a shot at removing use of descr in #7790. I copied your test, too.

Is that all right?

@jzwinck

jzwinck commented Jun 30, 2016

Copy link
Copy Markdown
Contributor Author

Closing in favor of #7790. Thanks @ahaldane .

@jzwinck jzwinck closed this Jun 30, 2016
@ahaldane

Copy link
Copy Markdown
Member

Thanks for finding the bug and identifying the problem code @jzwinck !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants