Add WCS 1D (including multi-D flux) writer in default_loaders

[dhomeier]

In #625 and related discussion on Slack the lack of a builtin writer for wcs1d-fits (writing the flux data as an image array) came up again.
This PR implements such an initial generic writer for saving a Spectrum1D to the (primary) IMAGE_HDU in a FITS file.

• Writing as wcs1d-fits only works for Spectrum1D objects that have a fully functional WCS (specifically having a spectrum.wcs.to_fits method; it's up to the user to provide this.

• The writer also supports on-the-fly change of flux units or dtype (e.g. to save space by setting dtype='f4') via kwargs; the same options have been added to tabular-fits, but may need some discussion (this implementation adds separate kwargs [fw]type for the dtypes of flux and spectral_axis, and there are no tests yet).

• wcs1d-fits actually happily accepts 2D flux arrays, and the resulting files can be opened e.g. in SAOImageDS9, but Spectrum1D cannot read them back in, as there is no unambiguous way to handle the WCS-constructed spectral axis. Deferring this to a separate PR or issue on handling multi-D spectra.

• Also following the discussion on Slack I have added auto-identification functionality one write to tabular-fits; from what I understood this is of limited use, as it can basically just check if the file naming scheme conforms to the rules. For wcs1d it would be helpful to check for a working wcs, but since the spectrum object is not passed to the identifier, I see no way to implement this.

[dhomeier]

Cannot really consistently reproduce the doc build failure (or the warning for that matter) locally:

Warning, treated as error:
1426 /home/travis/build/astropy/specutils/docs/api/specutils.SpectralCoord.rst:42:<autosummary>:1:py:obj reference target not found: Quantity
1427 Sphinx Documentation subprocess failed with return code 2

It initially showed up with Python3.7 + Sphinx 2.4.4, but not with Python3.8 + Sphinx 2.2.1.; after updating the latter to Sphinx 2.4.4 as well, the warning once popped up as well, but never since (always removing the _build and api subdirs in between)...

[dhomeier]

Worked out a way how to load 2D spectra on a common spectral axis (i.e. a range of spectra in the 1. dimension with the 2. dimension the spectral axis).
Auto-identification for 2D is not enabled, since it creates ambiguous matches with most other wcs-based loaders (I tried to resolve this by assigning them different priorities, but that seems to be broken at a deeper level as indicated in #584).

[dhomeier]

Seems the way the reader handles additional dimensions could be extended ad infinitum; included cases up to ndim/NAXIS = 4 now with the tests, I don't know if there are general use cases or interest in higher dimensions.

[dhomeier]

I have rebased since #637 introduced a number of non-trivial merge conflicts.

Main open questions remaining to be settled on my part are

1. Should the flux dimension wcs1d_fits_loader accepts remain limited to 4, any other number, or allow unlimited dimensions?

2. Are the naming conventions of the unit and dtype keywords for the writers sensible?

[nmearl]

I think we might want to rename this to something like image-fits/image_fits_writer (analogous to tabular) -- wcs1d should be implicit it because it's on a Spectrum1D writer, and I think we might want to express explicitly that it's writing to an images vs a table.

[dhomeier]

Technically there are two characteristic properties – storing the flux spectrum as a 1D image, and encoding the spectral axis as wcs.WCS. But I agree the first one is probably more informative to the user, and this might make the documentation on the different loader types clearer or easier to write.

[dhomeier]

So once I have updated both identifiers it would be probably be a good moment to go ahead with the renaming. Are you more in favour of FITS IMAGE, FITS BINTABLE, [FITS] IRAF; or rather IMAGE-FITS etc. @nmearl?

(I think FITS... would have the advantage that all three formats would be listed together, following ASCII and ECSV in the current loaders).

[nmearl]

This looks great other than the comment on having the identifier functions accept hdulist objects.

[dhomeier]

I had half forgotten this was still pending when updating the identifier functions in #708. I am adding the hdulist handling here as well, should be straightforward to merge if the new PR needs to be rebased – had to update both identifiers anyway since Travis is rebasing on master and otherwise test_2slaq_lrg_loader_science_and_sky will fail.

[nmearl]

Looks good, thanks @dhomeier!

[dhomeier]

Thanks @nmearl – a bit of explanation on the hdu kwargs for the writers: the overall default for writing is still tabular-fits, as it is the most general and flexible format. However if hdu=0 is specified, wcs1d becomes the default, since the primary HDU only accepts IMAGE data. Currently that kwarg has no other purpose in tabular_fits_writer; ideally it should support writing to a HDU of choice just as wcs1d_fits_writer now does, but since astropy's Table.write does not support any HDU selection, this will require some additional work at the io.fits level, so it's better left to a new PR. Then an updating mode (writing only to the specific HDU in an existing file) could also be tackled.

[nmearl]

ideally it should support writing to a HDU of choice just as wcs1d_fits_writer now does, but since astropy's Table.write does not support any HDU selection, this will require some additional work at the io.fits level, so it's better left to a new PR. Then an updating mode (writing only to the specific HDU in an existing file) could also be tackled.

Can you open an issue that points this out specifically, with the idea that it'll be tackled in a future PR (and link to this PR for context)?