-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Numpy 1.14 style changes #6959
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
Numpy 1.14 style changes #6959
Changes from all commits
306d053
b61ece7
c00f873
673da91
9841c15
3087769
6a6272f
8b935fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,8 +34,9 @@ class NDSlicingMixin: | |
| >>> import numpy as np | ||
| >>> mask = np.array([True, False, True, True, False]) | ||
| >>> nd2 = NDDataSliceable(nd, mask=mask) | ||
| >>> nd2[1:3].mask | ||
| array([False, True], dtype=bool) | ||
| >>> nd2slc = nd2[1:3] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid showing a bool array since |
||
| >>> nd2slc[nd2slc.mask] | ||
| NDDataSliceable([3]) | ||
|
|
||
| Be aware that changing values of the sliced instance will change the values | ||
| of the original:: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,7 +163,6 @@ def test_initialize_from_fits_with_data_in_different_extension(tmpdir): | |
| def test_initialize_from_fits_with_extension(tmpdir): | ||
| fake_img1 = np.random.random(size=(100, 100)) | ||
| fake_img2 = np.random.random(size=(100, 100)) | ||
| new_hdul = fits.HDUList() | ||
| hdu0 = fits.PrimaryHDU() | ||
| hdu1 = fits.ImageHDU(fake_img1) | ||
| hdu2 = fits.ImageHDU(fake_img2) | ||
|
|
@@ -279,7 +278,7 @@ def test_setting_uncertainty_with_array(ccd_data): | |
|
|
||
| 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) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, this made a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I already fixed that downstream (astropy/ccdproc#579) but forgot that the test had been copied over... |
||
| ccd_data.uncertainty = np.random.random(size=(3, 4)) | ||
|
|
||
|
|
||
| def test_to_hdu(ccd_data): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import os | ||
|
|
||
| from ..scripts import showtable | ||
| from ...utils.compat import NUMPY_LT_1_14 | ||
|
|
||
| ROOT = os.path.abspath(os.path.dirname(__file__)) | ||
| ASCII_ROOT = os.path.join(ROOT, '..', '..', 'io', 'ascii', 'tests') | ||
|
|
@@ -28,11 +29,20 @@ def test_info(capsys): | |
| def test_stats(capsys): | ||
| showtable.main([os.path.join(FITS_ROOT, 'data/table.fits'), '--stats']) | ||
| out, err = capsys.readouterr() | ||
| assert out == ('<Table length=3>{0}' | ||
| ' name mean std min max {0}' | ||
| '------ ------- ------- ---- ----{0}' | ||
| 'target -- -- -- --{0}' | ||
| ' V_mag 12.8667 1.72111 11.1 15.2{0}').format(os.linesep) | ||
| if NUMPY_LT_1_14: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We really shouldn't be testing float output!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can maybe use another table, could you check the output with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed about moving the discussion to #6962 , so the current change is fine by me. |
||
| expected = ('<Table length=3>{0}' | ||
| ' name mean std min max {0}' | ||
| '------ ------- ------- ---- ----{0}' | ||
| 'target -- -- -- --{0}' | ||
| ' V_mag 12.8667 1.72111 11.1 15.2{0}') | ||
| else: | ||
| expected = ('<Table length=3>{0}' | ||
| ' name mean std min max {0}' | ||
| '------ --------- --------- ---- ----{0}' | ||
| 'target -- -- -- --{0}' | ||
| ' V_mag 12.866668 1.7211105 11.1 15.2{0}') | ||
|
|
||
| assert out == expected.format(os.linesep) | ||
|
|
||
|
|
||
| def test_fits(capsys): | ||
|
|
@@ -49,12 +59,18 @@ def test_fits_hdu(capsys): | |
| showtable.main([os.path.join(FITS_ROOT, 'data/zerowidth.fits'), | ||
| '--hdu', 'AIPS OF']) | ||
| out, err = capsys.readouterr() | ||
| assert out.startswith( | ||
| ' TIME SOURCE ID ANTENNA NO. SUBARRAY FREQ ID ANT FLAG STATUS 1\n' | ||
| ' DAYS \n' | ||
| '-------- --------- ----------- -------- ------- -------- --------\n' | ||
| '0.144387 1 10 1 1 4 4\n' | ||
| ) | ||
| if NUMPY_LT_1_14: | ||
| assert out.startswith( | ||
| ' TIME SOURCE ID ANTENNA NO. SUBARRAY FREQ ID ANT FLAG STATUS 1\n' | ||
| ' DAYS \n' | ||
| '-------- --------- ----------- -------- ------- -------- --------\n' | ||
| '0.144387 1 10 1 1 4 4\n') | ||
| else: | ||
| assert out.startswith( | ||
| ' TIME SOURCE ID ANTENNA NO. SUBARRAY FREQ ID ANT FLAG STATUS 1\n' | ||
| ' DAYS \n' | ||
| '---------- --------- ----------- -------- ------- -------- --------\n' | ||
| '0.14438657 1 10 1 1 4 4\n') | ||
|
|
||
|
|
||
| def test_csv(capsys): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1209,7 +1209,7 @@ def get_delta_ut1_utc(self, iers_table=None, return_status=False): | |
| >>> t = Time(['1961-01-01', '2000-01-01'], scale='utc') | ||
| >>> delta, status = t.get_delta_ut1_utc(return_status=True) | ||
| >>> status == TIME_BEFORE_IERS_RANGE | ||
| array([ True, False], dtype=bool) | ||
| array([ True, False]...) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, ugly, but don't know what else to do... |
||
| """ | ||
| if iers_table is None: | ||
| from ..utils.iers import IERS | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,8 @@ without the need to download a large ephemerides file:: | |
| >>> with solar_system_ephemeris.set('builtin'): | ||
| ... jup = get_body('jupiter', t, loc) # doctest: +REMOTE_DATA +IGNORE_OUTPUT | ||
| >>> 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) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not completely sure why this didn't fail before: the new baryvel has z component of geovel != 0.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| (136.91116209, 17.02935409, 5.94386022)> | ||
|
|
||
| Above, we used ``solar_system_ephemeris`` as a context, which sets the default | ||
| ephemeris while in the ``with`` clause, and resets it at the end. | ||
|
|
||
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 changed the example such that it is
9 - 1/2**10and thus exactly representable.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 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?
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 think this is actually OK:
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 had8.999999(and with numpy 1.14, that is what you get out; older versions are the ones that are problematic).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.
Alright, you're the numpy expert here, so I'll just go with the flow.