Fixes for Python 3.7 by saimn · Pull Request #7617 · astropy/astropy

saimn · 2018-07-02T09:21:26Z

Running the test suite on Python 3.7 with a few optional dependencies (the ones easily installable, but not scipy or matplotlib).
With these fixes:

============== 2 failed, 9905 passed, 787 skipped, 70 xfailed in 351.16 seconds ===============

The annoying one is that overriding astropy's __dir__ attribute with a list no more works, not sure if it was really supported before ...

One remaining failure with FitsTime, a rounding error, but I don't know if using isclose would be adequate here (@mhvk, @taldcroft ?):

astropy/io/fits/tests/test_fitstime.py .............F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

self = <astropy.io.fits.tests.test_fitstime.TestFitsTime object at 0x7fef164aaa20>
table_types = <class 'astropy.table.table.Table'>

    @pytest.mark.parametrize('table_types', (Table, QTable))
    def test_io_time_read_fits_location(self, table_types):
        """
            Test that geocentric/geodetic observatory position is read
            properly, as and when it is specified.
            """
        # Datetime column
        c = fits.Column(name='datetime', format='A29', coord_type='TT',
                        time_ref_pos='TOPOCENTER', array=self.time)
    
        # Observatory position in ITRS Cartesian coordinates (geocentric)
        cards = [('OBSGEO-X', -2446354), ('OBSGEO-Y', 4237210),
                 ('OBSGEO-Z', 4077985)]
    
        # Explicitly create a FITS Binary Table
        bhdu = fits.BinTableHDU.from_columns([c], header=fits.Header(cards))
        bhdu.writeto(self.temp('time.fits'), overwrite=True)
    
        tm = table_types.read(self.temp('time.fits'), astropy_native=True)
    
        assert isinstance(tm['datetime'], Time)
        assert tm['datetime'].location.x.value == -2446354
        assert tm['datetime'].location.y.value == 4237210
        assert tm['datetime'].location.z.value == 4077985
    
        # Observatory position in geodetic coordinates
        cards = [('OBSGEO-L', 0), ('OBSGEO-B', 0), ('OBSGEO-H', 0)]
    
        # Explicitly create a FITS Binary Table
        bhdu = fits.BinTableHDU.from_columns([c], header=fits.Header(cards))
        bhdu.writeto(self.temp('time.fits'), overwrite=True)
    
        tm = table_types.read(self.temp('time.fits'), astropy_native=True)
    
        assert isinstance(tm['datetime'], Time)
        assert tm['datetime'].location.lon.value == 0
        assert tm['datetime'].location.lat.value == 0
>       assert tm['datetime'].location.height.value == 0
E       AssertionError: assert -2.1723632570150343e-10 == 0
E        +  where -2.1723632570150343e-10 = <Quantity -2.17236326e-10 m>.value
E        +    where <Quantity -2.17236326e-10 m> = <EarthLocation (6378137., 0., 0.) m>.height
E        +      where <EarthLocation (6378137., 0., 0.) m> = <Time object: scale='tt' format='fits' value=['1999-01-01T00:00:00.123456789(TT)' '2010-01-01T00:00:00.000000000(TT)']>.location

astropy/io/fits/tests/test_fitstime.py:304: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
[11] > /home/simon/dev/astropy/astropy/io/fits/tests/test_fitstime.py(304)test_io_time_read_fits_location()
-> assert tm['datetime'].location.height.value == 0
   6 frames hidden (try 'help hidden_frames')
(Pdb++) tm['datetime'].location
<EarthLocation (6378137., 0., 0.) m>
(Pdb++) tm['datetime'].location.lon
<Longitude 0. deg>
(Pdb++) tm['datetime'].location.lat
<Latitude 0. deg>
(Pdb++) tm['datetime'].location.height
<Quantity -2.17236326e-10 m>

Full log here: https://gist.github.com/saimn/88e5120119363ff53590e900ea76142a

Python 3.7.0 (default, Jun 28 2018, 18:00:41) [GCC 8.1.1 20180531] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import astropy >>> dir(astropy) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'list' object is not callable

astropy-bot · 2018-07-02T09:21:38Z

Hi there @saimn 👋 - 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.

taldcroft · 2018-07-02T09:50:38Z

Hmm, annoying to have a regression like this, floating point operations should be stable.

From a practical perspective I can't see any problem in using isclose(.., rtol=0, atol=1e-9) # meters. At some level I'm almost surprised it was passing before given the transformation from geodetic to xyz back to geodetic.

mhvk · 2018-07-02T14:16:21Z

@saimn - agreed with @taldcroft that isclose is fine here, since there is a transformation between geodetic and geocentric implicitly done in EarthCoordinates (really a small design mistake, something I hope to correct at some point... #4261 and in particular #4261 (comment))

saimn · 2018-07-02T18:29:48Z

@mhvk @taldcroft - Ok, thanks for the input, I have updated the test with isclose.

mhvk

PR looks good; just a small comment on a comment.

mhvk · 2018-07-02T18:41:12Z


 from types import ModuleType as __module_type__
-# Clean up top-level namespace--delete everything that isn't in __dir__
+# Clean up top-level namespace--delete everything that isn't in __dir_inc__


Must admit I find this auto-cleanup bad form, but that's somewhat out of scope of this PR!

Not really a cleanup, I had to change the use of __dir__ to list the attributes that shall not be deleted because setting a list to __dir__ no more works.

@saimn - I meant that I found the whole stanza rather bad form, not your change to it! It just seems that by being a bit more careful in the code itself, one can avoid introducing these unwanted things in the namespace (or delete them after their use). Anyway, as said, definitely out of scope here!

@mhvk - Ah ok! I agree with you, we could find a simpler way to keep the namespace clean.

mhvk · 2018-07-02T18:44:51Z

+        # py.test reads files with the 'U' flag, which is
+        # deprecated.
+        r"'U' mode is deprecated",
+        # inspect raises this slightly different warning on Python 3.6.


Should this be Python 3.6 and 3.7?

astrofrog · 2018-07-02T18:52:46Z

I think this needs a changelog entry, and we'll probably want to backport this to the LTS branch, so I think it should go in the next LTS release section. I know this might require a manual backport.

saimn · 2018-07-02T19:03:11Z

@astrofrog - it may too early to state that Astropy now supports Python 3.7, as there may be other errors with the other dependencies ?
👍 for the backport otherwise, it should not be too difficult hopefully.

mhvk · 2018-07-02T20:56:11Z

My sense would be to merge this as is -- well, there should be a run where all tests passed! -- and then open a new issue about python 3.7 support. For now, I added appropriate labels.

saimn · 2018-07-02T21:09:11Z

well, there should be a run where all tests passed!

It was passing for the previous commit, it is flagged as failed because it kept the astrobot status for the changelog. And I did not want to waste CI time with the latest commit while just editing a comment.

mhvk · 2018-07-02T21:15:16Z

OK, in that case I'm happy to merge! I'll add a note to #7416 to remind us to note python 3.7 support in the changelog.

Fixes for Python 3.7

bsipocz · 2018-07-11T13:05:22Z



-import collections
+import collections.abc


We can't have this for the 2.0.x backport. I'll remove this change from the backport, but anyway, expect that it won't cause any roubles given the last commit of the PR.

Yes, the warning is filtered for lxml, so should be fine.

Fixes for Python 3.7

saimn added 5 commits July 2, 2018 10:02

Fix deprecation warning when importing the ABCs from 'collections'

2d61303

Duplicate ignored warnings for Python 3.7

75e39a8

Remove use of inspect.formatargspec which is deprecated

204aa73

Ignore collections.abc warnings for lxml

8eee26b

Fix floating point error in FITS time test

e35668b

mhvk approved these changes Jul 2, 2018

View reviewed changes

Update comment about getargspec deprecation [ci skip]

dd353b1

mhvk added Refactoring no-changelog-entry-needed labels Jul 2, 2018

mhvk added this to the v2.0.8 milestone Jul 2, 2018

mhvk merged commit fdb6cec into astropy:master Jul 2, 2018

mhvk mentioned this pull request Jul 2, 2018

Adding python 3.7-dev to the travis matrix #7416

Closed

saimn deleted the py37 branch July 2, 2018 21:25

pllim added the Python3 label Jul 9, 2018

bsipocz pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #7617 from saimn/py37

a71c9c0

Fixes for Python 3.7

bsipocz reviewed Jul 11, 2018

View reviewed changes

bsipocz pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #7617 from saimn/py37

d422fd2

Fixes for Python 3.7

bsipocz pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #7617 from saimn/py37

cb57f54

Fixes for Python 3.7

bsipocz mentioned this pull request Jul 25, 2018

Add Python 3.7 testing to travis #7607

Merged

bsipocz pushed a commit that referenced this pull request Aug 2, 2018

Merge pull request #7617 from saimn/py37

0339212

Fixes for Python 3.7

pllim mentioned this pull request May 6, 2024

MNT: add astropy.system_info report helper function #16335

Merged

1 task

pllim mentioned this pull request Jan 3, 2025

ENH: ensure that dir(astropy) lists subpackages #17598

Merged

1 task done

Conversation

saimn commented Jul 2, 2018

Uh oh!

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

Uh oh!

Uh oh!

taldcroft commented Jul 2, 2018

Uh oh!

mhvk commented Jul 2, 2018

Uh oh!

saimn commented Jul 2, 2018

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

saimn Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

saimn Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Jul 2, 2018

Uh oh!

saimn commented Jul 2, 2018

Uh oh!

mhvk commented Jul 2, 2018

Uh oh!

saimn commented Jul 2, 2018

Uh oh!

mhvk commented Jul 2, 2018

Uh oh!

bsipocz Jul 11, 2018 • 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!

saimn Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astropy-bot Bot commented Jul 2, 2018 •

edited

Loading

bsipocz Jul 11, 2018 •

edited

Loading