MAINT: Improve float16 and float32 printing #28703
Conversation
|
@mhvk @tylerjereddy, PTAL, thanks! |
numpy/_core/tests/test_arrayprint.py
Outdated
| @@ -919,6 +919,21 @@ def test_floatmode(self): | |||
| a = np.float64.fromhex('-1p-97') | |||
| assert_equal(np.float64(np.array2string(a, floatmode='unique')), a) | |||
|
|
|||
| def test_gh_28679(self): | |||
There was a problem hiding this comment.
Here and below, I'd tend to pytest.mark.parametrize("value, expected", [np.half(...), "[999. 999.]"), ..., but no big deal.
There was a problem hiding this comment.
I was following the style used in other places in the file. Will keep this as is for now ...
There was a problem hiding this comment.
As others have requested to update this to parametrize, I will work on that.
There was a problem hiding this comment.
I added one stress testing-related comment where I'm not fully convinced just yet. I'm sure Marten knows more than I about the rest of the analysis and his comments look sensible to me in the other places (+1 for parametrization I think).
I'm going to go ahead and close gh-28696--I think it has served its purpose to refresh the discussion and refocus this to a solution the team seems to like better (and I think I agree, modulo my query about consistency).
There was a problem hiding this comment.
I think this looks good, so do my remaining comment only if there is a need to edit anything else.
I guess one larger question is whether there should be a what's-new fragment. Since this might affect downstream doctests, perhaps that is a good idea. What do others think? (it would be doc/release/upcoming_changes/28703.change.rst, I think; see README.rst in that directory).
numpy/_core/arrayprint.py
Outdated
| @@ -990,11 +990,15 @@ def fillFormat(self, data): | |||
|
|
|||
| # choose exponential mode based on the non-zero finite values: | |||
| abs_non_zero = absolute(finite_vals[finite_vals != 0]) | |||
|
|
|||
| # consider data type while deciding the max cutoff for exp format | |||
There was a problem hiding this comment.
If there is another round, then might as put this in the if statement. Indeed, in principle one can do the assignment with the walrus,
if len(abs_non_zero := absolute(finite_vals[finite_vals != 0])) != 0:
exp_cutoff_max = ...
|
Do we need a new revision of the legacy array printing format? Definitely needs a release note, also needs a ping to the mailing list probably, just to alert of possible downstream doctesting noise with a note to configure doc builds to use the legacy print format in doctests. |
|
Yes, you are right, we definitely should allow people to get the old format back by setting |
|
So we want to fallback to old behavior in case of: numpy.set_printoptions(legacy="2.2") correct? |
|
2.2 or older, yeah |
|
I confirmed that the property-based test passes for 16- and 32-bit types here now. As an aside, Marten and I both asked for parametrized testing and I'd prefer if reviewer comments could be marked as resolved by the reviewers since that change was declined by author and then marked resolved. Not a big deal, but also not really a great reason for push back on it either, and certainly would prefer if the maintainer resolved it. |
I've unmarked it. Since people feel strongly I'll update the tests to use parameterized testing (maybe tomorrow). Pushing another incremental commit to add legacy support. |
|
I believe I have resolved all outstanding review comments, PTAL. Also, while looking into legacy support I found get_printoptions() is broken for legacy mode 2.1 on latest. I've fixed that and added a sanity test for all supported legacy print modes with get_printoptions. |
|
Is there anything else that needs to be looked into before we merge this @ngoldbaum ? |
|
I'll wait for the other reviewers to give this a look. Keep in mind that this won't be available to new users until NumPy 2.3, so there's no particular rush to get this merged before the first RC goes out. I added the 2.3 milestone so this won't get dropped. |
Thanks to everyone for your feedback/suggestions! |
This patch fixes #28679.
Summary of changes: Update the max exponent cutoff for float16 and float32 (and corresponding complex) types to avoid weirdness when float types can no longer exactly represent integers.
Before the change:
print(np.half(65504)) # 65500
After the change:
print(np.half(65504)) # 6.55e+04
For a detailed background on the motivation, please check the linked issue.