Numpy 1.14 style changes by mhvk · Pull Request #6959 · astropy/astropy

mhvk · 2017-12-09T22:17:23Z

Numpy 1.14 is being branched, so it seemed time to ensure we're passing all tests.

EDIT: commits are by module - feel free to just look at relevant commits! Check-list for review:

An annoyance is that bool arrays are now represented without dtype=bool (since that is obvious from the content), which meant that every time this was done in doctest, I had to add ..., which is rather ugly.

Furthermore, the repr of MaskedArray has become much more logical, but that means our doctests cannot work on both new and old - hence, I just skipped those (after adjusting to the new style).

astropy-bot · 2017-12-09T22:17:25Z

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 labeled 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-12-09T22:56:31Z

@mhvk - This being milestoned to 2.0.3 means that I can go ahead and backport #6090, right?

mhvk · 2017-12-09T23:06:02Z

Ah, I had forgotten about that earlier discussion! I think if we can backport this without too much trouble, then, yes, let's do it (given that 2.0 is LTS).

bsipocz · 2017-12-09T23:54:32Z

The backport was surprisingly easy, there were some minor conflict only (and I hope git didn't mess up the automated ones).

mhvk · 2017-12-10T00:43:05Z

This now should have everything alright except for the new problems introduced by #6859.

taldcroft · 2017-12-10T01:13:01Z

-   15.0
-   17.0
-   15.7
+  >>> print(obs_by_name['mag_b'].groups.aggregate(np.mean))  # doctest: +SKIP


I looked at this on my cell phone 30 minutes ago and these values looked horrible (like 15.000000000002) and there was a doctest FLOAT_COMPARE. Now it's just skipped. What is the actual now?

They do look horrible on numpy 1,14 - I think this is due to a change in how scalars are represented - they don't necessarily go through arrayprint any more, but rather always are represented such that the value is uniquely defined. I'm going to raise a separate issue for that, but here just want to be sure tests will continue to pass when the 1.14rc hits pypi.

mhvk · 2017-12-10T01:55:21Z

      <TD>268435456</TD>
      <TD>922337203685477</TD>
-      <TD>8.9999990000000007</TD>
+      <TD>8.9990234375</TD>


I changed the example such that it is 9 - 1/2**10 and thus exactly representable.

I think "8.9999990000000007" was designed for a purpose (perhaps to test floating point precision), so I am not comfortable with this change. Is there no way to make test pass without changing the file?

I think this is actually OK:

In [1]: 8.9999990000000007 == 8.999999 Out[1]: True

Previously, numpy just put out a large number of digits, so that one was sure that a number was well-represented, but it didn't not check it could be done with fewer; now it puts out just the right number (see above).

It also seems clear the "truth" was generated once, and is now tested against. The actual input file (regression.xml) does in fact had 8.999999 (and with numpy 1.14, that is what you get out; older versions are the ones that are problematic).

Alright, you're the numpy expert here, so I'll just go with the flow.

mhvk · 2017-12-10T01:55:55Z

        >>> nd2 = NDDataSliceable(nd, mask=mask)
-        >>> nd2[1:3].mask
-        array([False,  True], dtype=bool)
+        >>> nd2slc = nd2[1:3]


Avoid showing a bool array since dtype=bool has disappeared.

mhvk · 2017-12-10T01:56:26Z


 def test_setting_uncertainty_wrong_shape_raises_error(ccd_data):
    with pytest.raises(ValueError):
-        ccd_data.uncertainty = np.random.random(size=2 * ccd_data.shape)


Oops, this made a 100,100,100,100 array (which is a bit much for my 2GB laptop)

Yeah, I already fixed that downstream (astropy/ccdproc#579) but forgot that the test had been copied over...

mhvk · 2017-12-10T01:56:55Z

-                   '------ ------- ------- ---- ----{0}'
-                   'target      --      --   --   --{0}'
-                   ' V_mag 12.8667 1.72111 11.1 15.2{0}').format(os.linesep)
+    if NUMPY_LT_1_14:


We really shouldn't be testing float output!

We can maybe use another table, could you check the output with astropy/io/ascii/tests/t/simple_csv.csv ? (I don't have time to install numpy 1.14 right now, maybe later today or tomorrow):

❯ showtable astropy/io/ascii/tests/t/simple_csv.csv --stats <Table length=2> name mean std min max ---- ---- --- --- --- a 2.5 1.5 1 4 b 3.5 1.5 2 5 c 4.5 1.5 3 6

I think this is a case where we should be testing non-nice floating point values. The whole point of showtable is creating human-readable output that is informative and "pleasant". So using real-world values is important to catch formatting issues that our users will end up seeing.

@taldcroft, @saimn - yes, although it may be that we have to define what is pleasant ourselves rather than rely on numpy doing it for us. But let's discuss in #6962, as it is somewhat orthogonal to the point of this PR, which is simply to get things to pass with numpy 1.14.x.

Agreed about moving the discussion to #6962 , so the current change is fine by me.

mhvk · 2017-12-10T01:57:15Z

            >>> delta, status = t.get_delta_ut1_utc(return_status=True)
            >>> status == TIME_BEFORE_IERS_RANGE
-            array([ True, False], dtype=bool)
+            array([ True, False]...)


Yes, ugly, but don't know what else to do...

mhvk · 2017-12-10T01:57:46Z

        """

-        filename = tmpdir.join(self.filename).strpath
+        filename = str(tmpdir.join(self.filename))


This allows one to call tests from ipython without having a tmpdir (no idea where to import it from)

@mhvk I just noticed that there are 5 other instances in this file also using .strpath (lines 23, 28, 33, 42, and 54). Do those need to be changed also?

A quick search reveals that there are a lot of tests across astropy that use tmpdir .strpath. Is a larger PR needed?

Yes, you're right, we should probably change all. It may be easiest if I remove this commit from the PR, since it is really not numpy-dev related.

mhvk · 2017-12-10T01:58:25Z

  >>> jup  # doctest: +FLOAT_CMP +REMOTE_DATA
-  <SkyCoord (GCRS: obstime=2014-09-22 23:22:00.000, obsgeoloc=( 3949481.689878457, -550931.9118838,  4961151.73733447) m, obsgeovel=( 40.1745933,  288.00078051,  0.) m / s): (ra, dec, distance) in (deg, deg, AU)
-    ( 136.91116201,  17.02935408,  5.94386022)>
+  <SkyCoord (GCRS: obstime=2014-09-22 23:22:00.000, obsgeoloc=(3949481.68990863, -550931.91188162, 4961151.73733451) m, obsgeovel=(40.15954083, 287.47876693, -0.04597867) m / s): (ra, dec, distance) in (deg, deg, AU)


Not completely sure why this didn't fail before: the new baryvel has z component of geovel != 0.

yeah, that's weird... I'm seeing the same as you, and that has nothing to do with numpy... maybe it's some weird quirk of the FLOAT_CMP? Anyway, looks fine to me

mhvk · 2017-12-10T01:59:42Z

-           fill_value = 1e+20)
-    <BLANKLINE>
+    >>> 2 * np.ones(3) * ccd_masked   # one return value will be masked  # doctest: +SKIP
+    masked_array(data=[2.0, 4.0, --],


Don't know how to get doctest to pass both old and new format...

mhvk · 2017-12-10T02:00:29Z

    >>> ndd4.mask
-    array([False,  True, False], dtype=bool)
-
-or even a masked Quantity::


I don't think we should encourage this in any way! Also, it looks very different in numpy 1.14 (probably a buglet)

Thanks for this feedback!

pllim

I am not comfortable with changing actual file values for votable tests.

pllim · 2017-12-11T16:21:25Z

      <TD>268435456</TD>
      <TD>922337203685477</TD>
-      <TD>8.9999990000000007</TD>
+      <TD>8.9990234375</TD>


I think "8.9999990000000007" was designed for a purpose (perhaps to test floating point precision), so I am not comfortable with this change. Is there no way to make test pass without changing the file?

MSeifert04 · 2017-12-11T16:55:52Z

I agree that io.fits looks good. I also basically approve the changes made in nddata, but I hope that either @crawfordsm or @mwcraig could also have a look.

larrybradley · 2017-12-11T17:05:20Z

        """

-        filename = tmpdir.join(self.filename).strpath
+        filename = str(tmpdir.join(self.filename))


pllim · 2017-12-11T21:07:07Z

      <TD>268435456</TD>
      <TD>922337203685477</TD>
-      <TD>8.9999990000000007</TD>
+      <TD>8.9990234375</TD>


Alright, you're the numpy expert here, so I'll just go with the flow.

eteq

LGTM as far as coordinates are concerned

eteq · 2017-12-11T21:57:08Z

  >>> jup  # doctest: +FLOAT_CMP +REMOTE_DATA
-  <SkyCoord (GCRS: obstime=2014-09-22 23:22:00.000, obsgeoloc=( 3949481.689878457, -550931.9118838,  4961151.73733447) m, obsgeovel=( 40.1745933,  288.00078051,  0.) m / s): (ra, dec, distance) in (deg, deg, AU)
-    ( 136.91116201,  17.02935408,  5.94386022)>
+  <SkyCoord (GCRS: obstime=2014-09-22 23:22:00.000, obsgeoloc=(3949481.68990863, -550931.91188162, 4961151.73733451) m, obsgeovel=(40.15954083, 287.47876693, -0.04597867) m / s): (ra, dec, distance) in (deg, deg, AU)


yeah, that's weird... I'm seeing the same as you, and that has nothing to do with numpy... maybe it's some weird quirk of the FLOAT_CMP? Anyway, looks fine to me

mwcraig

The nddata pieces of this look goo to me. Thanks for taking this on!

mwcraig · 2017-12-12T02:56:28Z

    >>> ndd4.mask
-    array([False,  True, False], dtype=bool)
-
-or even a masked Quantity::


Thanks for this feedback!

taldcroft

👍

bsipocz · 2017-12-12T11:01:57Z

@mhvk - can you rebase? Also I think this got approval from each relevant submodule lead or deputy, so ready to go.

Use a double that is exactly representable: 9 - 1/2**10 instead of 8.999999 which gets represented as 8.999999000007 (or so) on numpy <1.14.

In the process, changed one test that took a lot of memory as it created a random array of shape 100,100,100,100. Also removed all mention of masked Quantities, as these really are not supported, and should not be encouraged to be used in any way.

Note: cannot do MaskedArray sensibly, so just skipping those.

bsipocz · 2017-12-12T17:33:51Z

passing and approved, so I'm merging this now!

bsipocz · 2017-12-12T17:34:29Z

Enormous thanks @mhvk for keeping us compatible with the latest and greatest and even the future numpy!

Numpy 1.14 style changes

mhvk added Docs io.fits io.votable nddata numpy-dev stats table time labels Dec 9, 2017

mhvk added this to the v2.0.3 milestone Dec 9, 2017

mhvk added the no-changelog-entry-needed label Dec 9, 2017

mhvk force-pushed the numpy-dev-style-changes branch from e4cd13d to 4438383 Compare December 9, 2017 23:04

mhvk mentioned this pull request Dec 10, 2017

Add a showtable script #6859

Merged

mhvk force-pushed the numpy-dev-style-changes branch from 4438383 to 601ecbc Compare December 10, 2017 00:41

mhvk force-pushed the numpy-dev-style-changes branch from 601ecbc to 0fd8b13 Compare December 10, 2017 00:59

taldcroft reviewed Dec 10, 2017

View reviewed changes

mhvk force-pushed the numpy-dev-style-changes branch from 0fd8b13 to afdd9eb Compare December 10, 2017 01:45

mhvk commented Dec 10, 2017

View reviewed changes

mhvk force-pushed the numpy-dev-style-changes branch from 21e1597 to 92e3c3c Compare December 11, 2017 15:22

mhvk requested a review from adrn December 11, 2017 15:25

pllim reviewed Dec 11, 2017

View reviewed changes

pllim mentioned this pull request Dec 11, 2017

Some tests in io.votable failing on numpy-dev build #6850

Closed

larrybradley approved these changes Dec 11, 2017

View reviewed changes

larrybradley mentioned this pull request Dec 11, 2017

test of fits2bitmap (orientation) fails #6960

Closed

MSeifert04 requested review from crawfordsm and mwcraig December 11, 2017 17:14

pllim approved these changes Dec 11, 2017

View reviewed changes

eteq approved these changes Dec 11, 2017

View reviewed changes

mwcraig approved these changes Dec 12, 2017

View reviewed changes

taldcroft approved these changes Dec 12, 2017

View reviewed changes

saimn approved these changes Dec 12, 2017

View reviewed changes

mhvk added 8 commits December 12, 2017 11:56

Style changes for io.fits - mostly needing FLOAT_CMP.

306d053

Numpy style change in unified.rst

b61ece7

Avoid rounding problems in io.votable tests.

c00f873

Use a double that is exactly representable: 9 - 1/2**10 instead of 8.999999 which gets represented as 8.999999000007 (or so) on numpy <1.14.

Numpy 1.14 style changes for nddata

673da91

In the process, changed one test that took a lot of memory as it created a random array of shape 100,100,100,100. Also removed all mention of masked Quantities, as these really are not supported, and should not be encouraged to be used in any way.

Numpy style changes for stats

9841c15

Note: cannot do MaskedArray sensibly, so just skipping those.

Style changes for time

3087769

style changes for table

6a6272f

Style changes/corrections for coordinates docs.

8b935fb

mhvk force-pushed the numpy-dev-style-changes branch from 92e3c3c to 8b935fb Compare December 12, 2017 16:56

bsipocz merged commit 515fa9b into astropy:master Dec 12, 2017

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

Merge pull request #6959 from mhvk/numpy-dev-style-changes

468d275

Numpy 1.14 style changes

keflavich mentioned this pull request Jan 10, 2018

IRSA has changed URLs from http to https astropy/astroquery#1010

Merged

mhvk deleted the numpy-dev-style-changes branch December 20, 2018 19:56

Uh oh!

Uh oh!

Conversation

mhvk commented Dec 9, 2017 • edited by eteq Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

bsipocz commented Dec 9, 2017

Uh oh!

mhvk commented Dec 9, 2017

Uh oh!

bsipocz commented Dec 9, 2017

Uh oh!

mhvk commented Dec 10, 2017

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!

taldcroft Dec 10, 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!

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!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 9, 2017 •

edited by eteq

Loading

astropy-bot Bot commented Dec 9, 2017 •

edited

Loading

taldcroft Dec 10, 2017 •

edited

Loading