ENH: remove unneeded spaces in float/bool reprs, fixes 0d str by ahaldane · Pull Request #9139 · numpy/numpy

ahaldane · 2017-05-18T20:29:25Z

See discussion in #8983

This removes the leading space in the string representation of arrays of floating-point type if it is not needed (ie, all positive values), eg:

old behavior:

>>> np.array([1., 2., 3.])
array([ 1.,  2.,  3.])

New behavior:

>>> np.array([1., 2., 3.])
array([1., 2., 3.])

This noticebly improves the appearance of 0d arrays, so that np.array(1.0) prints as itself, instead of as np.array( 1.0) as happens after #8983.

It does the same for boolean arrays with only one element, so 0d boolean arrays print as array(True, dtype=bool) instead of array( True, dtype=bool).

(longfloats still have a space I think, but I would rather not tackle that here because I think it requires a big rewrite of longfloat formatter)

eric-wieser · 2017-05-18T20:36:58Z

numpy/core/arrayprint.py

Should be _nc.any

And probably has_sign not hassign

charris · 2017-05-19T15:46:40Z

This could break doc tests. Doc tests are evil, but some use them anyway...

ahaldane · 2017-05-19T19:51:35Z

@mhvk just for your info as we decide how to make the repr changes, I ran this PR though astropy tests, and it messes up a lot of tests:

79 failed, 11325 passed, 574 skipped, 68 xfailed, 1 xpassed, 1 pytest-warnings

Most of them are things like

Expected:
    <Quantity 15.0 m / s>
Got:
    <Quantity 15. m / s>

or

Expected:
    array([ 0. ,  0.4,  0.6,  0.8,  1. ])
Got:
    array([0. , 0.4, 0.6, 0.8, 1. ])

mhvk · 2017-05-19T19:54:34Z

@ahaldane - yes, but I've already got a PR that fixes those tests: astropy/astropy#6090; and am happy to fix things further if we go that route, as I think this PR made printing better in general. Just don't want to have to do it for each numpy version!

mhvk · 2017-05-19T20:52:42Z

@ahaldane - I clearly did not pay much attention to the PR in which you commented... Even up to redoing your work here in #9144 (now closed). In any case, my comment above stands, if slightly differently: I gladly fix the astropy doctests if we end up with more consistent numpy printing -- after all, I think that after this it is not going to change again... But ideally we do make all changes in one go for 1.14!

mhvk · 2017-05-21T17:41:13Z

If we try not to break too many things, one option for this would be to expose the sign option in set_printoptions, maybe having as options ' ', '+', '' (i.e., match what is used by the format mini-language). Though it would be a bit annoying that there is no way not to break compatibility if this is applied to both integers and float, since for integer the default would have to be '', while for floats it would be ' '.

ahaldane · 2017-05-24T03:07:30Z

Updated based on discussion in #9143.

Also, while we are annoying everyone by changing the spaces in array printing, we should try to do it as thoroughly as possible now to avoid any future changes. Specifically, I'd like to add another commit or two to 1. remove spaces from longfloat, and 2. I think the repr of np.array([100., 2., -1.]) can still be improved (see unit test for this): We should better account for whether the sign is on the longest value or not, currently this PR still has an unneeded space in this case.

(I also still need to update the release notes and other little things)

eric-wieser · 2017-05-24T14:16:55Z

numpy/core/tests/test_arrayprint.py

This case is identical to the one two above it, isn't it?

hmf not sure how that happened.. will fix

mhvk · 2017-05-24T15:55:46Z

numpy/core/tests/test_arrayprint.py

This comment is now wrong!

ahaldane · 2017-05-31T17:00:33Z

Updated.

I updated the treatment of signed values so the extra whitespace for the sign only appears if the longest elements have a sign. Release note and tests also updated.

I haven't updated the longfloat spacing, but I think I want to leave that for another possible PR. I'd like to get this one through so upstream projects can get their tests to stop complaining.

mhvk

Looks good, modulo a few text issues in the changelog, and an unneeded list construction (which was there already, but might as well get rid of it).

mhvk · 2017-05-31T17:06:00Z

doc/release/1.14.0-notes.rst

Idem, and also delete "an"

mhvk · 2017-05-31T17:07:36Z

numpy/core/arrayprint.py

Piece missing here. Change to "output correctly."?

mhvk · 2017-05-31T17:11:18Z

numpy/core/arrayprint.py

No need to construct a list inside max -- btw, unrelated to this PR, but this seems a really slow way for large arrays...

yeah I worried about that too, but then I worked out that data is going to be a small array.

data only contains the elements that are actually printed on the screen (ie, not the interior which is replaced by ...)

Ah, that makes sense.

mhvk · 2017-05-31T17:22:51Z

doc/release/1.14.0-notes.rst

Trivial, but capitalize first word.

mhvk · 2017-05-31T17:22:57Z

doc/release/1.14.0-notes.rst

ahaldane · 2017-05-31T17:39:24Z

numpy/core/arrayprint.py

Also, I think this line is unnecessary, will remove.

ahaldane · 2017-05-31T17:58:31Z

All right, reworked the trailing_nonzero part, hopefully the code is not too dense.

mhvk · 2017-05-31T18:31:08Z

Looks good to me.

mhvk · 2017-06-01T13:55:39Z

I'd be quite happy to merge this, as it seems a follow-up of gh-8983, and finishes making the print options much more logical. Obviously, this will break more doctests, but it does seem best to at least do it in one go. @eric-wieser, @charris: any objections to merging this?

eric-wieser · 2017-06-01T14:05:16Z

numpy/core/src/multiarray/scalartypes.c.src

As I think I said before, this isn't safe, since item can (and likely will for custom dtypes) return a copy of self, causing infinite recursion.

What actually falls back to this case anyway?
Seems to me that it would be better to just inherit object.__str__, making if obvious that custom dtype users have work to do.

Right, you are talking about this comment.

Currently, besides potential user-defined types, only numpy only void types fall back to this. But they no longer will after #8981.

I just tried your suggestion by simply removing gentype_repr and gentype_str, in the last commit pushed here.

Locally (not in the pushed commit) I also temporarily modified test_rational.c to remove the str and repr of rational methods as a check, and can confirm I get reasonable behavior:

>>> from numpy.core.test_rational import rational >>> a = np.array([1,2,3], dtype=rational) >>> a array([<rational object at 0x7f37f1b4e918>, <rational object at 0x7f37f1b4e918>, <rational object at 0x7f37f1b4e918>], dtype=rational)

I'm not sure how to implement a test of this without recreating a new test_rational type missing the str and repr, which seems a bit heavy-handed.

eric-wieser · 2017-06-01T14:39:05Z

numpy/core/arrayprint.py

Why did this decrease by one?

The simple answer is that there appeared to be an unnecessary space.

The more complex answer is that actually I'm not sure why that extra space was there... I'll take another look.

After investigation, I still don't understand why the extra space was there.

I count "6" extra characters more than the floating part and optional sign: x.e+xx where x are digits.

eric-wieser · 2017-09-21T07:02:24Z

numpy/core/arrayprint.py

Seems a bit odd that we're importing np directly for these, yet importing other stuff indirectly. Not something you need to fix in this PR though

Yes, looks like all the indirect imports can be replaced by the single np import - I just tried it, seems fine.

I won't do that in this PR though.

I've never understood quite how, or why, that worked. But it generally does.

I think it works because we never use or call any numpy functions or variables in this file at import time, only at runtime.

At the moment arrayprint is imported from the main__init__ file, the numpy module has been created but not populated, so we are allowed to import numpy, but not allowed refer to any of its functions or variables. However, we can use it inside of function definitions since by the time they are called the numpy module will have been populated.

(Actually, I see a few exceptions: At module import time we call multiarray.set_string_function and we refer to int_, float_, complex_, intc and longlong. Therefore these will still need to be directly imported).

Hmm, but it might be safer to leave these all the way they are: What if another module calls arrayprint functions at import time? Then we would have a problem. Ideally no other modules would, though.

(And this isn't theoretical: We recently had a problem because getlimits.pywas calling array2string at module import time, though we've now fixed it so it no longer does (#9113))

eric-wieser · 2017-09-21T07:03:06Z

numpy/core/arrayprint.py

Comment no longer matches code

ahaldane · 2017-09-21T14:36:35Z

Fixed the comment, plus the two doctrings describing the sign argument.

eric-wieser

LGTM! Thanks for the iteration.

Let's leave this a day or two to collect any final comments

eric-wieser · 2017-09-25T06:14:17Z

@mhvk: good to go from your perspective?

mhvk · 2017-09-25T14:15:41Z

Looked over it once more and, yes, good to go! Though we should ensure that LongFloat gets done before 1.14 as well...

ahaldane · 2017-09-25T15:56:17Z

I just tweaked the release note a little. I changed the title about Unneeded whitespace in float and bool array printing removed to Unneeded whitespace in float array printing removed and edited the note.

ahaldane · 2017-09-25T15:57:10Z

Otherwise, I checked over it again, I don't have more changes.

eric-wieser · 2017-09-26T01:18:29Z

I intend to put this in just to see who screams ;)

@charris, I think it's screaming time

ahaldane · 2017-09-26T01:26:38Z

Rebased for conflicts with #9688

charris · 2017-09-26T02:42:20Z

In it goes. Good job all.

charris · 2017-09-26T03:02:11Z

There look to be four remaining PR relevant to this. It would be helpful if folks could keep Allan's list current and close any PRs that are no longer relevant.

numpy 1.14 "fixes" the extra space in float array prints.(numpy/numpy#9139) Use char type to keep back/forward compatible.

Before: [[ 0. 0. 0. ] [ 0. 0. 0. ] [ 0. 0. 0. ]] With recent NumPy: [[0. 0. 0.] [0. 0. 0.] [0. 0. 0.]] See numpy/numpy#9139

eric-wieser reviewed May 18, 2017

View reviewed changes

ahaldane force-pushed the print_no_leading_space branch 2 times, most recently from 3d81fd9 to 53cfc62 Compare May 18, 2017 20:58

ahaldane added 01 - Enhancement component: numpy._core labels May 18, 2017

ahaldane mentioned this pull request May 19, 2017

ENH: fix str/repr for 0d-arrays and int* scalars #8983

Merged

mhvk mentioned this pull request May 19, 2017

WIP: correct maximum element space calculation in array2string #9144

Closed

mhvk mentioned this pull request May 21, 2017

WIP: MAINT: print 0d arrays using scalar str/repr #9143

Closed

eric-wieser reviewed May 24, 2017

View reviewed changes

mhvk mentioned this pull request May 24, 2017

Numpy dev tests failing horribly on Travis astropy/astropy#6114

Closed

mhvk reviewed May 24, 2017

View reviewed changes

numpy/core/tests/test_arrayprint.py Outdated

Copy link
Copy Markdown

Contributor

mhvk May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is now wrong!

rkern mentioned this pull request May 30, 2017

Additional space in 0d array repr #9185

Closed

ahaldane force-pushed the print_no_leading_space branch 2 times, most recently from dbec3f1 to 85a8c45 Compare May 31, 2017 16:43

ahaldane force-pushed the print_no_leading_space branch from 85a8c45 to 5767ac1 Compare May 31, 2017 17:17

mhvk reviewed May 31, 2017

View reviewed changes

ahaldane commented May 31, 2017

View reviewed changes

numpy/core/arrayprint.py Outdated

Copy link
Copy Markdown

Member Author

ahaldane May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this line is unnecessary, will remove.

ahaldane force-pushed the print_no_leading_space branch from 5767ac1 to d13d48e Compare May 31, 2017 17:55

eric-wieser reviewed Jun 1, 2017

View reviewed changes

ahaldane force-pushed the print_no_leading_space branch from fd4ea55 to 2cbe4c4 Compare September 21, 2017 00:12

eric-wieser reviewed Sep 21, 2017

View reviewed changes

numpy/core/arrayprint.py Outdated

Copy link
Copy Markdown

Member

eric-wieser Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment no longer matches code

ahaldane force-pushed the print_no_leading_space branch from 2cbe4c4 to 7be5048 Compare September 21, 2017 14:34

eric-wieser approved these changes Sep 22, 2017

View reviewed changes

ahaldane force-pushed the print_no_leading_space branch from 7be5048 to 5810349 Compare September 25, 2017 15:49

ahaldane added 6 commits September 25, 2017 21:20

ENH: remove unneeded spaces in float/bool reprs

8943851

TST: update tests to remove unneeded whitespace

3eed3a2

ENH: "pad_sign" option, also threshold and edgeitems

b2d709a

ENH: Simplify some code in arrayprint.py

95e9bfa

MAINT: arrayprint.py formatters no longer require ravel'd data

6bfef27

MAINT: fixups to legacy spacing in FloatFormat

710e032

ahaldane force-pushed the print_no_leading_space branch from 5810349 to 710e032 Compare September 26, 2017 01:25

charris merged commit d8be978 into numpy:master Sep 26, 2017

ahaldane mentioned this pull request Oct 24, 2017

WIP: MAINT: Rewrite LongFloatFormat, trim zeros in scientific notation output #9919

Closed

m-weigand mentioned this pull request Feb 20, 2018

fix doctests for numpy 1.14.0 gimli-org/pyGIMLi#133

Merged

xiaoqiangwang added a commit to CaChannel/CaChannel that referenced this pull request May 22, 2018

use char type waveform to test numpy retrieval

7334bd0

numpy 1.14 "fixes" the extra space in float array prints.(numpy/numpy#9139) Use char type to keep back/forward compatible.

bchretien added a commit to bchretien/roboptim-core-python that referenced this pull request Jul 4, 2018

Fix test for changes to NumPy

56ca346

Before: [[ 0. 0. 0. ] [ 0. 0. 0. ] [ 0. 0. 0. ]] With recent NumPy: [[0. 0. 0.] [0. 0. 0.] [0. 0. 0.]] See numpy/numpy#9139

Uh oh!

Conversation

ahaldane commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented May 19, 2017

Uh oh!

ahaldane commented May 19, 2017

Uh oh!

mhvk commented May 19, 2017

Uh oh!

mhvk commented May 19, 2017

Uh oh!

mhvk commented May 21, 2017

Uh oh!

ahaldane commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane commented May 31, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane commented May 31, 2017

Uh oh!

mhvk commented May 31, 2017

Uh oh!

mhvk commented Jun 1, 2017

Uh oh!

eric-wieser Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ahaldane commented May 24, 2017 •

edited

Loading

eric-wieser Jun 1, 2017 •

edited

Loading

ahaldane Jun 1, 2017 •

edited

Loading

ahaldane Jun 1, 2017 •

edited

Loading

ahaldane Sep 21, 2017 •

edited

Loading

ahaldane Sep 21, 2017 •

edited

Loading

eric-wieser left a comment •

edited

Loading

ahaldane commented Sep 25, 2017 •

edited

Loading