Fix auto-expansion of string columns when dealing with masked values#9623
Conversation
pllim
left a comment
There was a problem hiding this comment.
I'll defer to @taldcroft but I would like to note that one should inspect the numpy-dev job on Travis CI that is allowed to fail.
Thanks for the fix, @mhvk !
|
The allowed-to-fail job passed, so all looks OK. And of course both @taldcroft and I should have paid attention to that job in #9559... |
Yikes, that was pretty bad! |
|
I think that the two cases below should give consistent results and that the output dtype should be |
astropy/table/column.py
Outdated
| if values is np.ma.masked: | ||
| values = '' | ||
| else: | ||
| values = values.filled('') |
There was a problem hiding this comment.
| values = values.filled('') | |
| values = values.data |
|
Yes, that is indeed better; only for |
I think this is good, but I can't figure out how to reproduce this problem on master. Hint please? |
|
@pllim had set the IERS test to |
|
Yes, sorry. You need to remove this line here, which this PR has already done. astropy/astropy/utils/iers/tests/test_iers.py Line 254 in dc62579 |
|
In this PR, that line is already removed - so the fact that tests pass is relevant 😄 |
BTW I finally understand... (slow!). I think the "N/A" was a red-herring for me, but now I see that the original IERS problem is simply because of directly inserting |
|
No worries - I had exactly the same misunderstanding. |
|
BTW, I'm wondering if this needs a bug fix entry. It actually fixes a real bug in (previous) master unrelated to strings and #9559: |
|
Yes, you're right. Will create a quick PR with a changelog entry. |
|
See #9635 |
…on-changelog Belated changelog entry for #9623 [ci skip]
Fix auto-expansion of string columns when dealing with masked values
…on-changelog Belated changelog entry for #9623 [ci skip]
When inserting masked values, #9559 caused any string column to expand to allow "N/A" to be added. But this breaks
iers. Here, we ensure that masked values will never cause an expansion of string columns. Writing tests, it also seemed to make sense that to use the mask of whatever is inserted. I think this makes more sense (and seemingly there was no test for this).fixes #9600