Repository navigation
Emit string truncation warnings for MaskedColumn - #5819
Conversation
| value = np.ma.asanyarray(value, dtype=self.dtype.type) | ||
|
|
||
| # Turn value into a flat masked array with at least 1d | ||
| vals = np.ma.atleast_1d(value).flatten() |
There was a problem hiding this comment.
This will go wrong for >=2-d arrays of strings (which are of course not impossible...)
There was a problem hiding this comment.
Sorry to read so poorly -- I now see that all you are trying to do is to get the warning right.
There was a problem hiding this comment.
I improved the comments and code organization to make the intent of this section more clear.
| # array to the smallest length without initing from a Python list | ||
| # of strings. | ||
| if np.any(vals.mask): | ||
| vals = vals[~vals.mask].tolist() |
There was a problem hiding this comment.
This removes masked items, so changes the length of the list. Can this work?
There was a problem hiding this comment.
Oh, duh, this is only for the warning. Never mind....
|
Sorry for the noise in the comments. Might |
|
@mhvk - I used your idea of One question is if you have a better idea for determining the |
| # itemsize for a length=1 version of the string type. Remember itemsize | ||
| # is the total length in bytes for a single item. | ||
| value_str_len = np.char.str_len(value).max() | ||
| value_itemsize = value_str_len * np.array('', dtype=value.dtype.type).itemsize |
There was a problem hiding this comment.
It is bizarre that the number of elements cannot be grabbed more directly!
In any case, in the current version, I'd make the string '1', just in case zero-length strings become possible (which is planned; numpy/numpy#8970).
Alternatively, I guess one could do
self_max_len = int(self.dtype.str[2:])
if value_str_len > self_max_len:
There was a problem hiding this comment.
Can we always be sure that '1' (or any ASCII character) will be part of any future arbitrary string dtype? That was my motivation for using an empty string. I thought about your second solution as well. Even though I already used that for making the warning message, it still strikes me as bad form to parse the .str attribute like that for actual code logic. Agreed that there really should be a better way!
Anyway, I'll do something to avoid using the zero-length string.
| # - Make version of ``value`` that is flat masked array with at least 1d | ||
| # - Get rid of any masked elements. | ||
| # - If there are any vals then check for string truncation | ||
| vals = np.ma.atleast_1d(value).flatten() |
There was a problem hiding this comment.
I think the following simplification would work, no?
value =np.ma.asanyarray(value, dtype=self.dtype.type, ndmin=1)
self._check_string_truncate(value.filled(''))
|
Humm, looks like |
|
FYI, if you are tempted to use |
|
Should this be a good enough reason to drop support for python 3.3 for astropy 2.0? The route via lists that you had earlier could be quite bad for very large arrays. In another PR, we have also been wondering about dropping numpy 1.7... See #6000. |
|
Based on initial #6000 discussion I'll see if this works for Py 3.3 + numpy 1.9+. |
|
OK, numpy 1.9 with python 3.3 and 3.4 passes tests. So if we can just update the Travis CI matrix accordingly, this should be good. |
|
Since #6006 was merged, this should now be all OK, so merging! |
Closes #5817.
This also implements a workaround for numpy/numpy#8624. If we think this is important enough to backport then this bit would need to go in a separate PR.