Add PyFITS as astropy.io.fits - #47

I know this is not ready for merging, but just wanted to confirm that this builds and the tests run on MacOS 10.6 with Python 2.7.

I noticed a few absolute imports, e.g. in test_hdulist.py

Skimmed through the general utilities and Python 3 stuff. There is some redundancy there with stuff in vo.table, less so with pywcs (which has a lot less Python code). My only comment would be to consider if it made sense to separate out the generic Python 3 compatibility stuff from the numpy-specific stuff -- but it's not a huge deal. I think it's probably easiest to tackle the cross-subproject refactoring and redundant code removal after merging this pull request.

@mdboom: Agreed on worrying about refactoring more after things are merged. As for where the numpy-specific fixes go, I'm fine putting it in a separate module. Though it's still worth stressing that these are Python3-specific fixes.

@astrofrog: Some of the absolute imports are intentional because I simply didn't like imports that had like four dots in them. And I figured that for tests maybe absolute imports aren't necessary since, after all, you're testing the interface rather than using it internally. I can fix it though.

I've also waffled about relative vs. absolute imports in tests. I think I converted everything to relative in wcs and vo here, but there's definitely an argument to be made for using absolute in tests. Probably a minor point.

This brings up one thing that also appears to be missing -- the "legacy" pyfits compatibility shim. That would be a great thing to have and test with existing code, as I think it will be important to the adoption of astropy. But again, that can be done post this pull request.

I actually wouldn't be against putting in the pyfits shim in this pull request. Which brings up another question which is what to do with deprecated stuff: My plan was to go ahead and remove all deprecated code since this is sort of a "fresh" start. But it might be better to leave it in for the first n releases for the sake of adoption.

Right now most of the things marked "deprecated" are just functions/methods that were given new PEP8-compliant names. However, there are some bigger changes incoming on one of my development branches...

My gut feeling is to leave deprecated stuff in with DeprecationWarnings for the first release of Astropy. For the second major release we remove it. (And "import pyfits" would raise a DeprecationWarning, too, obviously). I think it might be easier to say "hey, use astropy, then you have fewer packages to install" rather than "hey, use astropy, make a bunch of changes to your code, and then you have fewer packages to install". The first hit should be free ;)

I agree that leaving in deprecated stuff is reasonable. Make sure the deprecation warnings clearly state something like "future versions of astropy will remove these features". Given we have this plan of releasing a "preview" version, that one could perhaps have the compatibility stuff, and it could be removed in the later releases. Or, alternatively, allow an actual "compatibility mode" that you have to manually activate that allows old-style usage. That might be a pain though. We can figure it out later.

And my feeling on the tests is that they should always be relative: I'm pretty sure that if I try to run a test with absolute imports by going into the source directory and typing py.test at the command line, it runs the installed astropy rather than the one that's in that source directory, which is the behavior relative imports are intended to avoid. (I haven't tested this, though, so I'm not entirely sure py.test doesn't just figure it out somehow.)

I think its definitely a good idea to try to be consistent, and if the only reason is an aversion to seeing ..., I think consistency should take precedence.

On Fri, Nov 4, 2011 at 8:47 PM, Erik Tollerud <
[email protected]>wrote:

And my feeling on the tests is that they should always be relative: I'm
pretty sure that if I try to run a test with absolute imports by going into
the source directory and typing py.test at the command line, it runs
the installed astropy rather than the one that's in that source
directory, which is the behavior relative imports are intended to avoid.
(I haven't tested this, though, so I'm not entirely sure py.test doesn't
just figure it out somehow.)

In most cases (certainly anything with C extensions), going into the source
directory to run the tests (with relative imports) is not going to work.
Nor will it work under Python3 for code that requires 2to3. This is
perhaps an argument in favor of using absolute imports in tests -- though
perhaps it is confusing.

Right now, going into tests and running them always seems to work just fine... for example, I can run all the wcs tests by just cding to the wcs directory and doing py.test tests. It won't recompile, that's true, and I'm sure you're right that 2to3 won't work, but the tests do run. I regularly find myself doing this in pure python code, so it seems silly to not allow that use case (it also makes it easier to customize py.test by using shell aliases and the like).

It's not that big of a deal - more important I think is that we just agree on a consistent usage and try to stick with it so as to avoid weird, confusing bugs due to sometimes using the local and sometimes using the installed version.

I've got three test failures in test_image.py using Python 2.7.1 and pytest-2.1.2 on Linux.

I guess this must be known already, otherwise I can paste the errors. The output of the test session is:
3 failed, 1085 passed, 44 skipped in 7.34 seconds

I'm also getting three failing tests, with Python 2.7.2 on MacOS 10.6.8. I'm going to paste the failures here for the record:

=================================== FAILURES ===================================
_____________________ TestImageFunctions.test_verification _____________________

self = <astropy.io.fits.tests.test_image.TestImageFunctions object at 0x10462ba50>
capsys = <_pytest.capture.CaptureFuncarg instance at 0x1051883f8>

    def test_verification(self, capsys):
        # verification
        c = fits.Card.fromstring('abc= a6')
        c.verify()
        out, err = capsys.readouterr()
>       assert ('Card image is not FITS standard (equal sign not at column 8).'
                in err)
E       assert 'Card image is not FITS standard (equal sign not at column 8).' in u''

Library/Python/2.7/lib/python/site-packages/astropy-0.0dev_r254-py2.7-macosx-10.6-x86_64.egg/astropy/io/fits/tests/test_image.py:217: AssertionError
_________________________ TestImageFunctions.test_fix __________________________

self = <astropy.io.fits.tests.test_image.TestImageFunctions object at 0x1050c8f50>
capsys = <_pytest.capture.CaptureFuncarg instance at 0x1050ee5a8>

    def test_fix(self, capsys):
        c = fits.Card.fromstring('abc= a6')
        c.verify('fix')
        out, err = capsys.readouterr()
>       assert 'Fixed card to be FITS standard.: ABC' in err
E       assert 'Fixed card to be FITS standard.: ABC' in u''

Library/Python/2.7/lib/python/site-packages/astropy-0.0dev_r254-py2.7-macosx-10.6-x86_64.egg/astropy/io/fits/tests/test_image.py:226: AssertionError
________________ TestImageFunctions.test_verification_on_output ________________

self = <astropy.io.fits.tests.test_image.TestImageFunctions object at 0x10463aa10>
capsys = <_pytest.capture.CaptureFuncarg instance at 0x1050eefc8>

    def test_verification_on_output(self, capsys):
        # verification on output
        # make a defect HDUList first
        x = fits.ImageHDU()
        hdu = fits.HDUList(x) # HDUList can take a list or one single HDU
        hdu.verify()
        out, err = capsys.readouterr()
>       assert "HDUList's 0th element is not a primary HDU." in err
E       assert "HDUList's 0th element is not a primary HDU." in u''

Library/Python/2.7/lib/python/site-packages/astropy-0.0dev_r254-py2.7-macosx-10.6-x86_64.egg/astropy/io/fits/tests/test_image.py:471: AssertionError
============== 3 failed, 1085 passed, 44 skipped in 15.86 seconds ==============

This fork of astropy does not build on Python 3.2:

Traceback (most recent call last):
  File "setup.py", line 58, in <module>
    for package in setup_helpers.iter_setup_packages():
  File "/Users/tom/tmp/astropy_erik/astropy/setup_helpers.py", line 80, in iter_setup_packages
    module = import_module(name)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/__init__.py", line 124, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/_bootstrap.py", line 807, in _gcd_import
    _gcd_import(parent)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/_bootstrap.py", line 819, in _gcd_import
    loader.load_module(name)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/_bootstrap.py", line 436, in load_module
    return self._load_module(fullname)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/_bootstrap.py", line 141, in decorated
    return fxn(self, module, *args, **kwargs)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/importlib/_bootstrap.py", line 342, in _load_module
    exec(code_object, module.__dict__)
  File "/Users/tom/tmp/astropy_erik/astropy/io/fits/__init__.py", line 19, in <module>
    from . import column
  File "/Users/tom/tmp/astropy_erik/astropy/io/fits/column.py", line 32, in <module>
    NUMPY2FITS = dict([(val, key) for key, val in FITS2NUMPY.iteritems()])
AttributeError: 'dict' object has no attribute 'iteritems'

@eteq: running "py.tests tests" in "astropy.wcs" doesn't work for me. The import fails because the C extension is not there. Is there something you're doing differently?

@eteq @mdboom That's partly what ./setup.py test is for. It will make sure all your C extensions are compiled and importable. That runs just fine out of the source (in Python 2.x). I'm still planning on coming up with a hack to get it to work for 2to3'd code, but haven't worked on it yet.

@migueldvb @astrofrog The tests that are failing for you are ensuring that specific warning messages are being output. I need to modify the tests to just catch the warnings themselves. But this is still curious--could it be that on your systems the warnings are going to stdout instead of stderr? Or are you somehow running Python with all warnings turned off?

I don't think I'm running Python with warnings turned off (or at least I've never set it up that way). Let me know if you want me to run any snippet to diagnose the issue.

@astrofrog Then the only other thing I can think if is that your warnings are somehow going to stdout instead. A simple test you could try, if you're inclined is:

def test(capsys):
    import warnings
    warnings.warn('Warning')
    out, err = capsys.readouterr()
    capsys.close()
    print
    print 'out', out
    print 'err', err

Put that in a module and run it with py.test and see which output stream the warning went to. But either way, I need to rewrite those tests to capture the warnings instead of relying on the text being in stdout/err.

FWIW -- test_wcs.py has a test that does warning capturing.

I have run that test with py.test with the following session log:

========================================== test session starts ==========================================
platform linux2 -- Python 2.7.1 -- pytest-2.0.3
collected 1 items

test_err.py .

======================================= 1 passed in 0.01 seconds ========================================

I have installed the stock python 2.7 package from gentoo's portage tree
and did not change any configuration settings if I remember correctly.

On Mon, Nov 07, 2011 at 08:24:08AM -0800, Erik Bray wrote:

@astrofrog Then the only other thing I can think if is that your warnings are somehow going to stdout instead. A simple test you could try, if you're inclined is:

def test(capsys):
    import warnings
    warnings.warn('Warning')
    out, err = capsys.readouterr()
    capsys.close()
    print
    print 'out', out
    print 'err', err

Put that in a module and run it with py.test and see which output stream the warning went to. But either way, I need to rewrite those tests to capture the warnings instead of relying on the text being in stdout/err.

---

Reply to this email directly or view it on GitHub:
#47 (comment)

@migueldvb Sorry, I forgot to add you should run it with py.test -s test_err.py so that it outputs the print statements at the end of the test.

@iguananaut Thanks for the indication, the warning was sent to sdterr so
that seems to be configured correctly.

========================================== test session starts ==========================================
platform linux2 -- Python 2.7.1 -- pytest-2.0.3
collected 1 items 

test_err.py 
out 
err /home/miguel/tmp/tests/test_err.py:3: UserWarning: Warning
  warnings.warn('Warning')

.

======================================= 1 passed in 0.13 seconds ========================================

@mdboom @iguananaut - Are you using python setup.py develop to install astropy in developer mode? If you do that it puts the compiled stuff where needed to be found by the tests. I'm not sure if this was an intended "feature" or just chance that it makes the tests work, but it is convinient... But, again, it doesn't re-build them - @iguananaut is certainly right that the best way to deal with that is the setup.py test command. But for pure-python packages, I don't see why we wouldn't want this option available.

Is there some practical reason that absolute imports are better in this case (other than aesthetics)?

I always use develop mode. The fact that it makes compiled modules importable is very much an intended feature--it wouldn't be too useful without it (except for pure Python packages). I wrote the 'test' command to rebuild when necessary, I think.

As for imports, I've been thinking about that and am leaning toward making them all relative. If nothing else it seems safer just to be consistent regardless of aesthetic hangups.

I'm closing this pull request, in part because I renamed the branch that it's based on, and I don't think GitHub liked that. At this point astropy.io.fits is a bit closer to merging too, so I think a new discussion/review is in order soon. Unless someone beats me to it, the new pull request will be #102.