BUG: fix AttributeError on accessing object in nested MaskedArray. #15949
Conversation
|
Sorry I'm really new to Travis-CI. I tried to check the details of the run but it seems like everything passed. I'm wondering why the check is still considered pending? Is there anything I could fix here? Thanks! |
numpy/ma/core.py
Outdated
| # only initialize to empty array if it dout does not | ||
| # have ._fill_value before. If it has, do not change | ||
| # the original behavior | ||
| dout._fill_value = np.array([], dtype) |
There was a problem hiding this comment.
This doesn't strike me as correct, _fill_value should always be a 0d array, this is a 1d array.
There was a problem hiding this comment.
Oops sorry I'll look into that!
There was a problem hiding this comment.
I suspect you want
| dout._fill_value = np.array([], dtype) | |
| dout._fill_value = np.empty((), dtype) | |
| dout._fill_value[()] = self._fill_value[indx] |
There was a problem hiding this comment.
Yes thank you! I'll try to add some tests for this in the next commit!
numpy/ma/tests/test_extras.py
Outdated
| def test_masked_all_with_dtype_str(self): | ||
| # Test masked_all works with nested array with dtype of an 'object' | ||
| # refers to issue #15895 | ||
| my_dtype = np.dtype([('b', [('c', object)], (1,))]) |
There was a problem hiding this comment.
The other issue case I identified above is when the dtype is my_dtype = np.dtype([('b', (object, (1,)))])
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
|
Hi this is really my first time contributing to an open-source project! I'm wondering if it's possible for these changes to be accepted? If the code and tests are not good enough I'll definitely try to modify something to it! |
numpy/ma/core.py
Outdated
| @@ -3305,7 +3305,8 @@ def _scalar_heuristic(arr, elem): | |||
| # which should have shape () to avoid breaking several | |||
| # methods. There is no great way out, so set to | |||
| # first element. See issue #6723. | |||
| if dout._fill_value.ndim > 0: | |||
| dtype = self._fill_value.dtype[indx] | |||
| if dtype.ndim > 0: | |||
There was a problem hiding this comment.
Is this change necessary?
I'm tempted to add assert isinstance(self._fill_value, np.ndarray) before this line.
There was a problem hiding this comment.
Sorry I'm kinda confused here, so if the _fill_value is a string instead of ndarray, are we suppose to throw an error or change it to a 0darray?
There was a problem hiding this comment.
_fill_value should never be anything other than a 0d array. If it's somehow a string here, then something else has a bug, and we should track down that bug.
There was a problem hiding this comment.
Ah I see, guess I was not understanding the expected behavior correctly.
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Hi @rena-xu, welcome! You're doing everything right, no worries. Review typically takes a while, looks like this one is getting close to merging. |
numpy/ma/core.py
Outdated
| @@ -3305,6 +3305,8 @@ def _scalar_heuristic(arr, elem): | |||
| # which should have shape () to avoid breaking several | |||
| # methods. There is no great way out, so set to | |||
| # first element. See issue #6723. | |||
| # something like gh-15895 has happened if this fails | |||
| assert isinstance(dout._fill_value, np.ndarray) | |||
There was a problem hiding this comment.
This seems wrong. If there is a way for dout._fill_value to ever not be an ndarray, then this should raise a ValueError, not an AssertionError. And it would be even better if there was a test to show how this could ever happen.
There was a problem hiding this comment.
it would be even better if there was a test to show how this could ever happen.
It can't ever happen, as far as a I know - but this code is bug-prone, and this makes it easier for us to work out the issue if it does fire.
We could use RuntimeError("internal numpy error") or similar if you prefer.
There was a problem hiding this comment.
+1. As far as I can tell we have no other asserts in the code, we do have RuntimeErrors for this kind of thing.
|
@rena-xu Requesting an update on this. Changes to assertion as suggested above should move this forward |
|
@vrakesh Sorry for the delay! I'll change this ASAP. |
|
Sorry I'm kinda new to this. I'm wondering if it's possible to re-run the azure-pipeline test? It failed due to an ImportError, which I feel like should not be cause by this code change. I was able to pass this part by running runtests.py locally, so I'm not sure if I can reproduce this error |
Yes, you're right - that failure is happening on other PRs as well, re-running isn't going to fix it right now. We're fine ignoring unrelated failures, so you're all good here. Time for a re-review of your most recent changes. |
numpy/ma/core.py
Outdated
| # something like gh-15895 has happened if this check fails. | ||
| # _fill_value should always be an ndarray. | ||
| if not isinstance(dout._fill_value, np.ndarray): | ||
| raise RuntimeError('Internal NumPy error.') |
There was a problem hiding this comment.
nit: can you move these lines up to line 3299? Where you've put them has separated the comment above from the code it describes.
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Closes #15895