New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure Masked scalars print the same way as regular array scalars #15451
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
Who should review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 1 question and 1 request for more detailed comments.
return self.maybe_mask_string(self.format_function(x.unmasked[()]), x.mask) | ||
|
||
@staticmethod | ||
def maybe_mask_string(string, mask): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered about separating it out at least if the function is used outside of the class, but looking at it again, realized I could just use the class, which for this bug-fix PR seems cleaner. So, this change is now reverted.
# Override to change special treatment of array scalars, since the numpy | ||
# code turns the masked array scalar into a regular array scalar. | ||
if a.shape == () and a.dtype.names is None: | ||
return MaskedFormat.maybe_mask_string(np.array_str(a.unmasked), a.mask) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would follow through to here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now use MaskedFormat
explicitly, with np.array_str
as the format function. That seems cleaner.
Eventually, I'd like to give the option of using different ways to indicate masked - in particular, with strikethrough! (but it is tricky since not all terminals support the utf-8 way of doing it; see line 965)
assert str(msa) == "[3.14159265 ———]" | ||
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | ||
assert str(msa[1]) == " ———" | ||
with np.printoptions(precision=3, floatmode="fixed"): | ||
assert str(msa) == "[3.142 ———]" | ||
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | ||
assert repr(msa) == "MaskedNDArray([3.14159265, ———])" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert str(msa) == "[3.14159265 ———]" | |
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | |
assert str(msa[1]) == " ———" | |
with np.printoptions(precision=3, floatmode="fixed"): | |
assert str(msa) == "[3.142 ———]" | |
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | |
assert repr(msa) == "MaskedNDArray([3.14159265, ———])" | |
# Test masking the array works as expected, including truncating digits | |
assert str(msa) == "[3.14159265 ———]" | |
# Test the digits are kept | |
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | |
# Or appropriately masked | |
assert str(msa[1]) == " ———" | |
# Test the precision can be changed temporarily. | |
with np.printoptions(precision=3, floatmode="fixed"): | |
assert str(msa) == "[3.142 ———]" | |
assert str(msa[0]) == "3.141592653589793" == str(sa[0]) | |
assert repr(msa) == "MaskedNDArray([3.14159265, ———])" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea. Implemented.
5e4510f
to
3dd8ada
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion to speed up by avoiding the lambda, otherwise 🚀
3dd8ada
to
3e07dc6
Compare
Description
This pull request is to ensured that
str(masked_array)
looks likestr(unmasked_array)
also for array scalars. Thus, like regular array scalars, the precision is ignored for float, and strings do not include extra quoting.Found in #15231
I think this should not be backported, since it is not nice if
str(anything)
changes in a bug-fix release. But I do consider it more of a bug-fix than an API change (though happy to change it if need be).