Numpy style changes #6090

mhvk · 2017-05-17T18:17:35Z

numpy ~~1.13~~ 1.14 changes the formatting of float scalars, to be identical to what is used for arrays. This is a good thing in principle, but breaks a lot of doctests. Furthermore, array2string now has the style keyword deprecated.

This PR hopefully addresses both issues.

I'm putting the milestone at 2.0, since it may not be worth backporting... EDIT: especially since the real change is for numpy 1.14, not 1.13.

mhvk · 2017-05-18T01:26:09Z

@bsipocz, @adrn - would you be able to have a look? It is mostly just keeping up with a change in numpy (one for the better, in my opinion) in which scalars now use the same formatting as values in an array.

mhvk · 2017-05-18T01:42:54Z

As this seems to have solved the numpy-1.13rc problems as well, I'm reverting the allowed failure for rc.

fixes #6089

adrn

Left some comments, but mainly questions (sorry for asking so many!).

adrn · 2017-05-18T02:01:48Z

astropy/units/tests/test_quantity.py

+        if NUMPY_LT_1_14:
+            assert repr(self.scalarfloatq) == "<Quantity 1.3 m>"
+        else:
+            assert repr(self.scalarfloatq) == "<Quantity  1.3 m>"


Huh. I know you're probably just making changes to make it consistent with what numpy does, but seems strange that an extra space is added here?

The array style always has a place for the sign.

adrn · 2017-05-18T02:02:44Z

docs/coordinates/angles.rst

    <Angle 3.5 rad>
    >>> np.sin(a / 2)  # doctest: +FLOAT_CMP
    <Quantity 0.479425538604203>
-    >>> a == a


Why did this get removed?

I couldn't get doctest to think both ( True and (True were the same thing, and I thought this example was sufficiently trivial (and repeated below) that I could just remove it.

adrn · 2017-05-18T02:03:59Z

docs/modeling/models.rst

    >>> g = Gaussian1D(amplitude=1, mean=0, stddev=1)
-    >>> g
-    <Gaussian1D(amplitude=1.0, mean=0.0, stddev=1.0)>
+    >>> g  # doctest: +FLOAT_CMP


If FLOAT_CMP is needed here, what do the actual repr's end up looking like?

For old numpy, 1.0, for new numpy 1.

adrn · 2017-05-18T02:05:41Z

docs/nddata/subclassing.rst


    >>> ndd = NDDataMaskBoolNumpy([1,2,3])
-    >>> ndd.mask = True
+    >>> ndd.mask = True, False, True


I know this is valid, but maybe we want to make it [True, False, True] to be more explicit? This might look strange to new users.

adrn · 2017-05-18T02:06:24Z

docs/units/equivalencies.rst

  >>> import numpy as np
-  >>> np.exp((1j*0.125*u.cycle).to('', equivalencies=u.dimensionless_angles()))  # doctest: +FLOAT_CMP
-  <Quantity (0.7071067811865476+0.7071067811865475j)>
+  >>> np.exp((1j*0.125*u.cycle).to('', equivalencies=u.dimensionless_angles()))  # doctest: +SKIP


Why is this skipped now?

New numpy removes the parentheses around the complex number; I didn't know how to get doctest to accept both as valid...

bsipocz · 2017-05-18T11:07:50Z

astropy/units/function/logarithmic.py

-        >>> u.Magnitude(15.)
-        <Magnitude 15.0 mag>
-        >>> u.Magnitude(10.*u.count/u.second)
+        >>> u.Magnitude(12.5)  # doctest: +FLOAT_CMP


Are all these FLOAT_CMP necessary? The float format usually isn't messed up when no arithmetic is done, isn't it?

I think this particular one is actually not necessary -- the real reason is to ensure that nnn.0 and nnn. are treated as being the same -- and here I changed to nnn.5 for that reason.

bsipocz · 2017-05-18T11:16:40Z

@mhvk - Looks good to me.

A bit more FLOAT_CMP were added to the docstring than I like, but I guess it's better to be on the safe side. I wish none of these doctest comments were shown in the python interpreter when asking for help, but that's a completely separate upstream issue.

mhvk · 2017-05-18T12:46:40Z

@bsipocz - good point about the docstrings; I'll see if for those I can try a bit harder not to use FLOAT_CMP.

mhvk · 2017-05-18T15:04:49Z

This is OK, but let's hold off a little as there is some discussion in numpy/numpy#8983 (comment) about whether the effects of the change were all that good (though I'd guess that would only lead to more/other edits...)

bsipocz · 2017-06-26T12:30:52Z

@mhvk - I guess there is still no clear decision upstream, so I'm moving the milestone of this to the next bugfix release.

astropy-bot · 2017-07-28T17:35:34Z

Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here

bsipocz · 2017-10-16T20:31:32Z

@mhvk - Not backporting this would mean that the LTS version wont support 1.14. Do we really want that?

mhvk · 2017-10-18T00:21:42Z

@bsipocz - I think this is mostly up to you - do you think this is easy enough to backport?

mhvk · 2017-10-18T00:23:19Z

Note to whoever reviews: to make tests pass, I had to modify FLOAT_CMP to include eating up preceding spaces. It now is used so often that I wonder whether we should just enable it by default (like we already do with NORMALIZE_WHITESPACE. (This would be for a follow-up PR.)

bsipocz · 2017-10-18T01:06:32Z

@mhvk - if the rebase was relatively easy, then backporting shouldn't be too messed up either (I hope at least).

bsipocz · 2017-10-18T14:59:27Z

@mhvk - This looks uncontroversial to me, but if you think otherwise please assign someone to do the review.

bsipocz · 2017-10-18T15:01:01Z

Re: FLOAT_CMP - I think it's a good idea to have it on by default, the rst files and docstrings look rather messy with it. However I wonder whether we need to add another directive to switch it off, is there any place in the code that requires not using it?

mhvk · 2017-10-18T15:27:05Z

Agreed this should be uncontroversial, so maybe you can give the required OK? And then perhaps we move the FLOAT_CMP to a new issue.

bsipocz · 2017-10-18T15:29:23Z

I agree that it's better to move the FLOAT_CMP to a separate issue/PR.

bsipocz · 2017-10-18T15:29:41Z

Thanks very much @mhvk!

mhvk · 2017-10-18T15:35:47Z

Finally things should work with numpy-dev again!

Numpy style changes

https//:github.com/astropy/astropy/pull/6090

Numpy style changes

mhvk added no-changelog-entry-needed numpy-dev testing labels May 17, 2017

mhvk added this to the v2.0.0 milestone May 17, 2017

ahaldane mentioned this pull request May 17, 2017

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

Merged

mhvk mentioned this pull request May 18, 2017

Investigate numpy rc failures and remove allowed-failure in .travis.yml #6089

Closed

adrn reviewed May 18, 2017

View reviewed changes

bsipocz reviewed May 18, 2017

View reviewed changes

mhvk force-pushed the numpy-style-changes branch from a56b7fe to 56372a4 Compare May 18, 2017 13:49

mhvk mentioned this pull request May 19, 2017

ENH: remove unneeded spaces in float/bool reprs, fixes 0d str numpy/numpy#9139

Merged

bsipocz mentioned this pull request May 24, 2017

Numpy dev tests failing horribly on Travis #6114

Closed

mhvk force-pushed the numpy-style-changes branch from 56372a4 to d4e7536 Compare May 24, 2017 21:15

bsipocz modified the milestones: v2.0.1, v2.0.0 Jun 26, 2017

bsipocz modified the milestones: v2.0.1, v2.0.2 Jul 28, 2017

bsipocz mentioned this pull request Aug 28, 2017

Travis on merge-to-master is failing because of numpy-dev #6480

Closed

bsipocz modified the milestones: v2.0.2, v2.0.3 Sep 7, 2017

mhvk added 6 commits October 15, 2017 11:44

Numpy style changes for modeling.

94d7c99

Numpy style changes for io.

781a940

Numpy style changes for cosmology.

423d767

Numpy style changes for coordinates.

99526d6

Numpy style changes for convolution.

b66eced

Adapt FLOAT_CMP to treat a single preceding space as a sign.

9b7f675

mhvk force-pushed the numpy-style-changes branch from af2c87f to 9b7f675 Compare October 15, 2017 15:45

Continue to skip test with large numpy-version-dependent differences.

8105ad7

bsipocz approved these changes Oct 18, 2017

View reviewed changes

bsipocz merged commit 29a8a23 into astropy:master Oct 18, 2017

mhvk deleted the numpy-style-changes branch October 18, 2017 15:35

drdavella mentioned this pull request Oct 18, 2017

Port changes to FLOAT_CMP handling from astropy/master scientific-python/pytest-doctestplus#7

Closed

MSeifert04 mentioned this pull request Oct 19, 2017

Fix failures against numpy dev astropy/ccdproc#566

Merged

bsipocz mentioned this pull request Dec 9, 2017

Numpy 1.14 style changes #6959

Merged

8 tasks

bsipocz modified the milestones: v3.0.0, v2.0.3 Dec 9, 2017

bsipocz added a commit that referenced this pull request Dec 9, 2017

Merge pull request #6090 from mhvk/numpy-style-changes

c408737

Numpy style changes

drdavella added a commit to drdavella/pytest-doctestplus that referenced this pull request Nov 14, 2018

Port from astropy: fix float comparison logic...

35b3e95

https//:github.com/astropy/astropy/pull/6090

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020

Merge pull request astropy/astropy#6090 from mhvk/numpy-style-changes

0e47ecb

Numpy style changes

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 8, 2020

Merge pull request astropy/astropy#6090 from mhvk/numpy-style-changes

b516536

Numpy style changes

Uh oh!

Numpy style changes #6090

Numpy style changes #6090

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mhvk commented May 18, 2017

Uh oh!

mhvk commented May 18, 2017

Uh oh!

adrn 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!

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!

bsipocz commented May 18, 2017

Uh oh!

mhvk commented May 18, 2017

Uh oh!

mhvk commented May 18, 2017

Uh oh!

bsipocz commented Jun 26, 2017

Uh oh!

astropy-bot bot commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Oct 16, 2017

Uh oh!

mhvk commented Oct 18, 2017

Uh oh!

mhvk commented Oct 18, 2017

Uh oh!

bsipocz commented Oct 18, 2017

Uh oh!

bsipocz commented Oct 18, 2017

Uh oh!

bsipocz commented Oct 18, 2017

Uh oh!

mhvk commented Oct 18, 2017

Uh oh!

bsipocz commented Oct 18, 2017

Uh oh!

bsipocz commented Oct 18, 2017

Uh oh!

mhvk commented Oct 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mhvk commented May 17, 2017 •

edited

Loading

astropy-bot bot commented Jul 28, 2017 •

edited

Loading