Repository navigation
Address issue #8995 by ignoring BINARY2 mask bits for string values. - #9057
Conversation
6782c9a to
e69f057
Compare
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Thank you! Need a rebase to get rid of some of the CI failures. Also, seems like test still needs some work? Please see comments.
| output = io.BytesIO() | ||
| astropy_table.write(output, format='votable') | ||
|
|
||
| # TODO - Construct test cases for unicodeChar strings and fixed length |
There was a problem hiding this comment.
Do you have plans to follow up on this as part of this PR?
There was a problem hiding this comment.
I thought more about this and realize that the TODO is a larger homework assignment for me to better review/understand the existing test coverage. That is outside the scope of this PR, so I filed issue #9433 and removed the TODO comment.
| # Ignore the mask for string columns (see issue 8995) | ||
| for i, f in enumerate(fields): | ||
| if f.datatype == 'char' or f.datatype == 'unicodeChar': | ||
| row_mask_data[i] = False |
There was a problem hiding this comment.
Do you think we should emit warning when this happens?
There was a problem hiding this comment.
I don't think so. For the data consumer, they are getting the behavior they expect for empty strings (the behavior they would get from the other serializations), so a warning would be confusing.
For the data provider, there is a better case for the warning, since setting the null bit for the string may not have meant what they thought. But the spec is pretty vague on this, so I'd like to get more clarification before issuing a warning. Two other VOTable validators I tried do not issue a message of any kind for this case.
|
Could you please rebase this? Also, let me know if there is a chance to finish it up in the next couple of days, otherwise feel free to remilestone to 4.0 |
|
@tomdonaldson , do you have time to wrap this up soon? Otherwise, we'll have to move the milestone. |
e69f057 to
992cab1
Compare
…alues on read.
Accept review suggestion prefix the conditional so that the rest of it doesn't need to execute nearly as often. Co-Authored-By: P. L. Lim <[email protected]>
f94a896 to
23cb42b
Compare
VOTables with BINARY2 serialization can mark values as null with a bit at the beginning of the data row. On read, each of these marked values was being stored as a numpy.ma.core.MaskedConstant. That makes sense for numerical values, but much less so for string values. Those MaskedConstants for strings then caused problems on writing the table back out.
This fix changes the reader behavior to ignore the BINARY2 null bits for string values.