BUG/TST: Fix for #6723 including test: force fill_value.ndim==0 #6728
Conversation
9f8f739 to
f667c81
Compare
| @@ -3121,6 +3121,13 @@ def __getitem__(self, indx): | |||
| if isinstance(indx, basestring): | |||
| if self._fill_value is not None: | |||
| dout._fill_value = self._fill_value[indx] | |||
| # If we're indexing a multidimensional field in a | |||
| # structured array (such as dtype("(2)i2,(2)i1")), | |||
There was a problem hiding this comment.
Would be clearer if written dtype("(2,)i2,(2,)i1"). Current success looks like a parse oddity.
fd68517 to
8d1ce6e
Compare
|
It's a messy situation, and this may be the best solution. Another option that comes to mind is that maskedarray could always broadcast the fill value to the data shape. Perhaps this can be achieved by a few judicious uses of |
|
@ahaldane Is it sure that such broadcasting is always possible? My gut feeling says it is, but my gut feeling is not very reliable. I'm still not clear what practical purpose it serves at all for a user to change the |
|
It's probably possible, but after thinking I'm pretty sure it will open up many new cans of worms and would take effort. For instance, we would have to worry about slicing the fill_value if the array gets sliced. I think this PR is a fair fix for this rare case, for now. If/when we end up reimplementing masked arrays to fix all sorts of issues like this (there have been suggestions to do so once I looked at the code in this PR, it looks good to me. I vote for merging. |
|
@ahaldane, just the side discussion.... But I tend to think right now that the "correct" solution from a future point of view would be to not allow it at all, each dtype should have a single mask value (of course only the last layer of a dtype has multiple dtypes inside). |
Fix issue numpy#6723. Given an exotic masked structured array, where one of the fields has a multidimensional dtype, make sure that, when accessing this field, the fill_value still makes sense. As it stands prior to this commit, the fill_value will end up being multidimensional, possibly with a shape incompatible with the mother array, which leads to broadcasting errors in methods such as .filled(). This commit uses the first element of this multidimensional fill value as the new fill value. When more than one unique value existed in fill_value, a warning is issued. Also add a test to verify that fill_value.ndim remains 0 after indexing.
8d1ce6e to
090e85e
Compare
|
Well, I don't think @seberg objected above, and looking over it again it seems like a reasonable band-aid. The most important part is just to warn the user if something strange is happening. Merging. |
…rray_fillvalue BUG/TST: Fix for #6723 including test: force fill_value.ndim==0
Fix issue #6723. Given an exotic masked structured array, where one of
the fields has a multidimensional dtype, make sure that, when accessing
this field, the fill_value still makes sense. As it stands prior to this
commit, the fill_value will end up being multidimensional, possibly with a
shape incompatible with the mother array, which leads to broadcasting
errors in methods such as .filled(). This commit uses the first element
of this multidimensional fill value as the new fill value.
Also add a test to verify that fill_value.ndim remains 0 after indexing.