Add PyFITS as astropy.io.fits

[embray]

I still need to rebase this on master and clean up a few things.

Also need to add support for the new config system--haven't decided whether to do that before or after merge.

[astrofrog]

I have three test failures on MacOS 10.6 with Python 2.7:

=========================================================== FAILURES ===========================================================
______________________________________ TestHeaderFunctions.test_verify_invalid_equal_sign ______________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x103a9fa10>
capsys = <_pytest.capture.CaptureFuncarg instance at 0x104146440>

    def test_verify_invalid_equal_sign(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''

astropy/io/fits/tests/test_header.py:335: AssertionError
_______________________________________ TestHeaderFunctions.test_fix_invalid_equal_sign ________________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x10417c990>
capsys = <_pytest.capture.CaptureFuncarg instance at 0x103f4e878>

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

astropy/io/fits/tests/test_header.py:343: AssertionError
________________________________________ TestImageFunctions.test_verification_on_output ________________________________________

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

    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''

astropy/io/fits/tests/test_image.py:218: AssertionError
====================================== 3 failed, 1310 passed, 48 skipped in 27.43 seconds ======================================

[eteq]

My mild preference would be to integrate the config stuff before merging, if you think it won't take too long.

[embray]

Those look like the same tests that were failing before...somehow the
stderr capture isn't picking up warning messages. I guess I still need to
change those tests to use warning capture instead.

[embray]

I kind of debated myself on that...but maybe you're right. The precedent we
want to set when adding packages is that they already fit into the existing
infrastructure.

Still need to do something about utility unification though...but that I
think I'd rather save for after this merge.

[mdboom]

@iguananaut: I've had similar issues with stderr capture under pytest, but the warning capture technique seems to work fine.

I agree about utility unification after the merge.

[embray]

Just strange that capturing warnings from stderr works on Linux but not on OSX. Suggests that on OSX Python doesn't send warnings to stderr by default, though a quick test on bond (one of our OSX build machines) suggests otherwise. So I'm not sure what's going on there...

[astrofrog]

Just for the record, the tests still fail, but with different errors:

===================================================== FAILURES =====================================================
________________________________ TestHeaderFunctions.test_verify_invalid_equal_sign ________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x104185c10>

    def test_verify_invalid_equal_sign(self):
        # verification
        c = fits.Card.fromstring('abc= a6')
        with warnings.catch_warnings(record=True) as w:
            c.verify()
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x104185cd0>, <warnings.WarningMessage object at 0x104185d10>, <warnings.WarningMessage object at 0x104185d50>])

astropy/io/fits/tests/test_header.py:321: AssertionError
_________________________________ TestHeaderFunctions.test_fix_invalid_equal_sign __________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x104185e90>

    def test_fix_invalid_equal_sign(self):
        c = fits.Card.fromstring('abc= a6')
        with warnings.catch_warnings(record=True) as w:
            c.verify('fix')
            fix_text = 'Fixed card to meet the FITS standard: ABC'
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x1041b4350>, <warnings.WarningMessage object at 0x1041b41d0>, <warnings.WarningMessage object at 0x1041b4590>])

astropy/io/fits/tests/test_header.py:330: AssertionError
__________________________________ TestImageFunctions.test_verification_on_output __________________________________

self = <astropy.io.fits.tests.test_image.TestImageFunctions object at 0x1041b4590>

    def test_verification_on_output(self):
        # verification on output
        # make a defect HDUList first
        x = fits.ImageHDU()
        hdu = fits.HDUList(x)  # HDUList can take a list or one single HDU
        with warnings.catch_warnings(record=True) as w:
            hdu.verify()
            text = "HDUList's 0th element is not a primary HDU."
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x103f3dc90>, <warnings.WarningMessage object at 0x103f3dd10>, <warnings.WarningMessage object at 0x103f3dd90>])

(this is with Python 2.7)

[astrofrog]

Interestingly, the same tests pass with Python 3.2

[mdboom]

To add another data point: On my Fedora 16 machine at home, all tests pass on both Python 2 and 3, so we're still maybe seeing something OS-X-specific here.

[mdboom]

Also -- you maybe already know this -- there seems to be a bad interaction between _fix_dtype and astropy.io.vo. vo builds a dtype as a list and passes that to the Numpy array constructor, but it seems _fix_dtype assumes it is a dtype object with a fields member.

[embray]

In this case it seems like more warnings are popping out than expected. I'll have to try it out on OSX and see what warnings are actually being output.

@mdboom I'll have to look into the _fix_dtype thing. I just need to make it more flexible.

[astrofrog]

@iguananaut - here are the failures with the warnings printed to stdout:

=============================================================== FAILURES ===============================================================
__________________________________________ TestHeaderFunctions.test_verify_invalid_equal_sign __________________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x1040d5a50>

    def test_verify_invalid_equal_sign(self):
        # verification
        c = fits.Card.fromstring('abc= a6')
        with warnings.catch_warnings(record=True) as w:
            c.verify()
            for wi in w:
                print wi.message
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x1040d5b50>, <warnings.WarningMessage object at 0x1040d5bd0>, <warnings.WarningMessage object at 0x1040d5c50>])

astropy/io/fits/tests/test_header.py:323: AssertionError
----------------------------------------------------------- Captured stdout ------------------------------------------------------------
Output verification result:
Card image is not FITS standard (equal sign not at column 8).
Card image is not FITS standard (unparsable value string: a6).
Note: PyFITS uses zero-based indexing.
___________________________________________ TestHeaderFunctions.test_fix_invalid_equal_sign ____________________________________________

self = <astropy.io.fits.tests.test_header.TestHeaderFunctions object at 0x1040d5e90>

    def test_fix_invalid_equal_sign(self):
        c = fits.Card.fromstring('abc= a6')
        with warnings.catch_warnings(record=True) as w:
            c.verify('fix')
            fix_text = 'Fixed card to meet the FITS standard: ABC'
            for wi in w:
                print wi.message
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x1040d5c10>, <warnings.WarningMessage object at 0x1040d5b10>, <warnings.WarningMessage object at 0x1040d5b90>])

astropy/io/fits/tests/test_header.py:334: AssertionError
----------------------------------------------------------- Captured stdout ------------------------------------------------------------
Output verification result:
Card image is not FITS standard (equal sign not at column 8).  Fixed card to meet the FITS standard: ABC
Card image is not FITS standard (unparsable value string: a6).  Fixed card to meet the FITS standard: ABC
Note: PyFITS uses zero-based indexing.
____________________________________________ TestImageFunctions.test_verification_on_output ____________________________________________

self = <astropy.io.fits.tests.test_image.TestImageFunctions object at 0x1040d5b50>

    def test_verification_on_output(self):
        # verification on output
        # make a defect HDUList first
        x = fits.ImageHDU()
        hdu = fits.HDUList(x)  # HDUList can take a list or one single HDU
        with warnings.catch_warnings(record=True) as w:
            hdu.verify()
            text = "HDUList's 0th element is not a primary HDU."
            for wi in w:
                print wi.message
>           assert len(w) == 1
E           assert 3 == 1
E            +  where 3 = len([<warnings.WarningMessage object at 0x1037e5b10>, <warnings.WarningMessage object at 0x1037e5b90>, <warnings.WarningMessage object at 0x1037e5c50>])

astropy/io/fits/tests/test_image.py:219: AssertionError
----------------------------------------------------------- Captured stdout ------------------------------------------------------------
Output verification result:
HDUList's 0th element is not a primary HDU.
Note: PyFITS uses zero-based indexing.
========================================== 3 failed, 1310 passed, 48 skipped in 24.32 seconds ==========================================

[embray]

Thanks for the debugging! Looks like it actually makes 3 warn() calls for a multi-line warning message. That's dumb--I need to fix that on the pyfits end.

[astrofrog]

All tests pass on Python 2.6, and 2.7 on MacOS 10.6.8. I still get one FITS failure and quite a few VO failures with Python 3.2. I don't seem to get the VO failures with the upstream master though. I'll email you the traceback (too long for pastebin).

[astrofrog]

The issue seems to be caused by the fact that _fix_dtype(dtype) expects a Numpy dtype, not just a list of fields and types which is also a valid way of representing a dtype when initializing an array.

[embray]

@astrofrog Sweet! Yeah, @mdboom pointed out the _fix_dtype thing to me. I've already fixed that in PyFITS and just need to merge it to astropy. What's the other FITS failure you're getting though?

[embray]

Nevermind, I see you sent me an e-mail. I'll have to look at it when I get back to work.

[astrofrog]

All tests now pass on MacOS 10.7 with all supported Python and Numpy versions.

[embray]

Huzzah \o/ Thanks again @astrofrog for setting up that Jenkins build for this branch--very useful.

[eteq]

Cool! Does that mean, pending a rebase, this is in principal ready to merged? I personally haven't looked at it too closely, but I'm curious if that means now we should? (at least for integration-with-astropy items - I certainly don't intend to review the entire code base :)

[embray]

@eteq I still need to add a handful of config options via the Astropy config system. There are also some environment variables, but I might leave them out for now. I was working on an environment variable patch for the config system a while back, but I got distracted and never got back to that branch... So I don't think I'll hold up fits over that.

[embray]

I think this is pretty much ready to go at this point.

In the process of giving one last look over the code I stumbled over all the design issues in PyFITS that I still want to fix. But they certainly won't all get fixed immediately so there's no reason to hold up Astropy for them. Though I'll still be continually merging changes from PyFITS, I don't think there's any reason to hold off much longer on merging this.

[astrofrog]

I can't review all the code (not sure if anyone can!) but from I can see, you've done a great job. I looked at the docs a bit, and one thing I think we might want to think about is whether we want to recommend common abbreviations for astropy components. For example, the following is a bit long to type:

>>> import astropy.io.fits
>>> hdulist = astropy.io.fits.open('input.fits')

but of course we could have 'standard' abbreviations (like numpy has with np), e.g.

>>> import astropy.io.fits as fits
>>> hdulist = fits.open('input.fits')

I'm sure we can deal with that in a future pull request, but just wanted to raise the point for now.

[embray]

@astrofrog That's a good point, and something that's crossed my mind too.. I was thinking of adding something in the docs to the tune of "Standard practice is to import the fits module with from astropy.io import fits" but I'm not quite sure where to put that..

[astrofrog]

@iguananaut I guess this applies to all components of Astropy, so maybe we should first merge this in, then we can decide on a consistent convention across packages. I like the idea of:

 from astropy.io import fits
 from astropy.io import vo
 from astropy import wcs

etc.

[eteq]

@astrofrog @iguananaut - I agree that I like the way @astrofrog suggests for doing imports - it seems easier to read, somehow.

@iguananaut - just so I understand what you meant in your earlier comment: you're saying that now all the configuration is via the astropy configuration system, and you're hoping to (post-merge) get the environment variable stuff working, or did you mean that you've left some stuff in this branch that still uses environment variables, and will switch that to the configuration system after the environment variable option is put in?

[eteq]

A few docs-related comments:

• Maybe change the "astropy.io.fits History" document title o "astropy.io.fits pre-Astropy History" or something like that? And perhaps @mdboom should do the same for vo and wcs' histories? Or do you intend to continue updating these even after merging with astropy?
• When I do python setup.py build_sphinx I get a bunch of documentation warnings, but if I do make html inside the docs directory, everything builds without complaint. Do any of you see the same behavior, or is this just a consequence of some sort of strange fiddling I've done with my python setup?
• I see you added an astropy.css file to docs/_static - what exactly is that for? And is it strictly necessary to make the pyfits docs look right?

[embray]

@eteq Okay, I'm seeing all these warnings with build_sphinx too. No idea why. I had only tested it by running make in the docs directory manually, which works fine for me.

I don't want to do anything with the History right now, since it will probably continue to be updated. I'll still be making changes in PyFITS and merging them into Astropy up through the first Astropy release, and probably beyond. It makes sense the way it is, since those changes will continue to originate from PyFITS at least for a while.

And yes, astropy.css contains some custom styles that are used in the PyFITS docs as well. There's also some LaTeX bits that I might port over at some point. I haven't tried doing a ps build of the Astropy docs yet...

[embray]

I think the problem with the build_sphinx command is that the docs are looking for classes in the astropy.io.fits namespace, but nothing is actually imported into that namespace when running setup.py because the package's __init__.py doesn't import anything into that namespace if setup_helpers.is_in_build_mode(). It's not clear to my why this isn't a problem for the io.vo package too.

[mdboom]

On the setup.py build_sphinx problem: io.vo works because it's importable without C extensions and the documentation pulls things from the modules they're defined in rather than their references in astropy/io/vo/__init__.py. wcs has the same problem I think we're seeing with fits, though even more pronounced because many of the docstrings only exist in the C extension.

In #117 (comment) we had discussed modifying build_sphinx so it would import from the build/lib.$PLATFORM tree, not the source tree. That should theoretically resolve this. What's the status on that?

[mdboom]

We might want to confirm that astropy.css plays with with the readthedocs css. Not worth holding up the pull for...

There's some tweaks in stsci_sphinxext that make the Parameters tables take up less horizontal space that I think would also be nice to include in astropy... Will try to work up a pull request for that at some point...

[embray]

@mdboom The stuff in astropy.css shouldn't have any effect. I defined a custom class for a figure I wanted formatted in a certain way, and that class is only used by the fits docs.

As for build_sphinx, running it out of the build/ dir would fix most of the problems. But it would still be necessary to amend setup_helpers.in_build_mode() to do something else when build_sphinx is running. I don't really want to change the API docs to use full module names, because all the documentation is designed so that users of the library can get everything they need directly from the astropy.io.fits module without going into any of the submodules. So I don't want to confuse the issue.

[mdboom]

We could just run build_sphinx in a separate process, as we already do with setup.py test.

Agreed on not changing the API docs -- I'm not sure how that problem is related -- or maybe I just haven't had enough caffeine today ;)

[embray]

@mdboom The API docs are a related problem, because if build AND build_sphinx are run as part of the same process, then by the time build_sphinx is run, the astropy.io.fits, astropy.wcs and other such packages will have already been imported but without their namespaces populated since in_build_mode() returns True. So when Sphinx goes about collecting objects in a module it can't actually find anything.

But running it in a subprocess, as you suggested, would solve that problem. Another possibility would be to have build_sphinx force a reimport of any astropy packages.

[eteq]

@mdboom @iguananaut - Regarding changing the docs to use astropy/build - I plan to do that once I get #115 working (as there would likely be conflicting changes between those), but #115 has taken a bit longer than I expected. If someone else wants to take a crack at it before I get that done, I could also rebase #115 against that.

Regardless, +1 on the subprocess idea; it should do the trick and consistency with setup.py test is a plus.

[astrofrog]

So is the plan to implement the change to the doc-building after this pull request is merged in? If so, is there anything else that needs addressing before we merge?

[embray]

Yeah I dunno--if we don't fix the docs first, merging this will break the docs. I don't really care too much right now given that it's only temporary. But that's just me.

[eteq]

There are lots of oddities in the way build_sphinx seems to build the docs that it might already be considered broken anyway, and this just solidifies it... so as far as the docs are concerned, I'd say its fine to merge this if its ready and later add the fix that generates the docs against astropy/build

[embray]

Okay then. By the end of today I will do one last rebase on trunk, and then merge \o/

[astrofrog]

Sounds good!

[eteq]

This is a huge merge! Good work, @iguananaut

[embray]

@eteq It actually seemed to break GitHub for a while. For at least an hour after the merge it wasn't properly displaying the source tree. I finally gave up and went home, but it seems to be fixed now.

[embray]

I'm going home for the day. Not feeling too great so I need to take care of myself.

To the remaining Astropy visitors, I'm sorry I didn't get to say bye properly but it was good seeing you, and I'm sure I'll see you around.

Erik