removing WCS

[astropy]

removing WCS

[eteq]

@profxj - one relevant thing you might not know: there is a plan afoot (as in hopefully to get a draft out this week) to publish a Python interface for a more general definition of "WCS" than "FITS-WCS". The "WCS" currently envisioned for specutils is that more general definition. That is to say: "WCS" in this context is to be thought of as "a rule to get from pixel space to physical space", not the more narrow "follow the FITS WCS standard".

Or do you mean you want no WCS as in "want to use pixels directly"? I fully agree that should be supported, but I think as a convenience it's going to be easier in the end to have a wcs attribute exist but have it be a simple linear y=x "wcs" (with a physical type of "pixel"). That would then be the default (automatically created) if no wcs is created. That's not implemented yet, but I think that was the plan once we have the more general wcs document to go.

Does this sound reasonable?

[profxj]

@eteq -- thanks for this update.

What I am advocating, as I have all along, is that WCS should be supported
but not required. And, if WCS is not used, then the wavelength values could/should
be stored as a simple numpy array (or equivalent). If a method then requires a
WCS, one will have to recast these numpy arrays into some y=x "wcs" that you mention.

The same holds for error values. One can get more astropy-complex and use
objects like StdDevUncertainty but this should not be the default (and definitely
not required). The error arrays we are accustomed to should be stored in
a light-weight numpy array.

[keflavich]

@profxj That's pretty much what we want too. The current implementation doesn't have the wcs=lambda x: y[x] version yet, but it's also not functional yet. We'll make sure that the APE clearly states that a complicated WCS object is not needed.

[profxj]

@keflavich -- Ok, but there is a big difference between a simple WCS object
and any WCS object being needed.

[nmearl]

But for a spectrum, WCS is always there, whether explicit or not, or whether a user is aware or not, as just a language for associating pixel space with world space. In the case of a simple numpy array, you're still implicitly stating that your WCS is a direct mapping of your pixel space to world space.

I think the fear is that inter-operability would suffer if a package developer using Spectrum1D objects had to also be aware of whether or not WCS machinery is implemented for it, depending on the needs of their users. At the minimum, I think a Spectrum1D object should support the API for WCS with the default implementation being functionally equivalent to having no WCS. Thus, a user does not have to be aware of it until it's needed, and package developers can assume that the interface for WCS is constant among all usages of a Spectrum1D object.

However, it's also completely possible to put the onus on package developers instead of us. Perhaps we could provide a simple WCS mixin that overrides methods of the inheriting class to use WCS instead of simple numpy arrays. But this also means we'd lose things like coordinate frame information, etc. in cases where WCS is not used.

I personally find the first implementation easier to support and maintain (both for us and potential users of the package), but could be swayed differently.

[profxj]

I'll admit I have a hard time parsing @nmearl 's comment which shows
I am not really a high-level developer. And, I appreciate this means I am not
viewing aspects of this like the rest of you (and therefore don't properly appreciate
what I am sure are good viewpoints). But let me remphasize my own viewpoint
which I believe is a practical one built from years of coding/analyzing astronomical
spectra. Which is:

1. We almost always store spectra as simple arrays (these days the wavelengths too)
2. Almost all of our methods tend to work on flux, wavelength, and error as simple
   arrays.

Given 1 and 2, I should prefer to have the Spectrum1D object store the data
as simple arrays. I might prefer straight numpy arrays, but I can surely
learn and deal with an NDData object. We can then wrap whatever level of complexity
that we want on top of those arrays.

As a bit of an aside, I do find it telling that both @dkirkby and I reached a similar approach
in our own work as regards packing the spectra in simple arrays (XSpectrum1D, speclite).
And, I'll add that it was the painful complexity of WCS and StdDevUncertainty that had
me flee the previous incarnation of specutils. Oh the hoops one had to jump through to
simply read in a spectrum..

[keflavich]

So we do have a "simple numpy array" WCS object here:
https://github.com/astropy/specutils/blob/master/specutils/spectra/wcs_wrappers.py#L10
but the implementation is not complete or tested and frankly I don't understand it yet (and I think I wrote it...). But the idea is to have some sort of WCS "object" that has a simple function pix_to_world = lambda pixel: coordinate[pixel], plus optional(?) metadata about frame information etc.

I summarized that off the top of my head so it almost certainly leaves out crucial details.

EDIT: and a key point of this is that we should make it trivially easy to instantiate a Spectrum object with a numpy array as the x-axis.

[keflavich]

This object: https://github.com/astropy/specutils/blob/master/specutils/spectra/spectrum_mixin.py#L66 is the spectral_axis, and it looks somewhat complicated, but in the case of having an array-of-values spectral axis, it should be equivalent to just having Spectrum.spectral_axis be a numpy array, but with a (tiny) bit of extra overhead.

[crawfordsm]

Yes, I should say that #2 would be the way that I envision it. There will be a very straight forward way to access wavelength, flux, and error. There very well might be a WCS working behind the scenes, but if we do not have that functionality for the user, then we have made the user facing aspect of the object overly complex.

…

On Tue, Sep 19, 2017 at 2:43 PM, Adam Ginsburg ***@***.***> wrote: This object: https://github.com/astropy/specutils/blob/master/ specutils/spectra/spectrum_mixin.py#L66 is the spectral_axis, and it looks somewhat complicated, but in the case of having an array-of-values spectral axis, it should be equivalent to just having Spectrum.spectral_axis be a numpy array, but with a (tiny) bit of extra overhead. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDk9GtXaY_wOCuZFT_p9otH0jSkhkbSks5skAtbgaJpZM4PbzF9> .

[keflavich]

@crawfordsm that "#2" linked to issue 2... you meant bullet 2 in #159 (comment)?

[eteq]

@profxj - just to be clear: totally understand your "been burned before" attitude here - I think you're right that this is the majority of users.

But we had a discussion about this just today (as you can guess from the comments...) which we're trying to resolve the conflicting needs, and one question from that: if you could create new Spectrum1D objects with a spectral_axis just as you said, access it as spectrum.spectral_axis (and the ways you listed in your item # 2), but have the actual underlying implementation be inside a .wcs object, is that OK? So basically this information would be in a .wcs object, but you could (and should) ignore that attribute completely unless you had to do something more complicated than accessing it as a spectral_axis?

[profxj]

Yes, I suspect that is a compromise that I'm willing to live with,
but with two conditions:
a. The underlying implementation be fast, e.g. the wavelength array
should not be re-created every time it is requested (i.e., you are
going to need to store an input array as an array in the WCS implementation)
b. I want to be able to (easily) ingest many 1D spectrum into
on Spectrum1D object. This is somewhat tangential to this WCS thread,
but I think rather related. The request is part convenience and part speed
(and fully a recognition that the era of 1 million 1D spectra at a time is upon us).

Anyhow, I did some fussing with the current Repo to get the basic functionality
that I want/need. This Notebook shows it (and you can see the few edits I've
made to achieve this if you like, but the coding is not elegant):

https://github.com/profxj/specutils/blob/strip_wcs/docs/nb/Fiddling_about.ipynb

ps. I have other add-ons in addition to the the error array I have tossed
in here (the source continuum, a mask/flag array), but
that surely deserves its own thread..

[eteq]

Re: @profxj's a) from #159 (comment) Ok, great - that should be just fine, as any WCS that's using tabular values as you like would have to store them as arrays anyway, and in that case the spectral_axis can just point straight there. More complex WCS can utilize caching to make it performant (that requires more work, but hopefully you don't need to care about that at all if you don't want more complex WCS).

Re: b) I think that's also OK, but it will be very useful to get your view on the specutils APE we're about to put out (hopefully early next week) - it hits a lot of the items in your notebook, but with some subtle differences. I think it will meet that need but it will be more useful to get your feedback directly there (will ping you as soon as it's out). Then once things are settled there we'll be able to implement changes in specutils itself.

[profxj]

Roger roger.

Taking all future discussion from here to the proper
APE place (where will that be?).

Closing this Issue.

[chummels]

I know this issue is closed, but I wanted to chime in as well. As one of the primary developers of Trident, (https://github.com/trident-project/trident/), a python suite for generating synthetic spectra, I also have concerns about requiring WCS in the spectral objects in astropy. Our synthetic spectra are generated from the virtual universes of computer simulations, so they lack any WCS information at all. It would be wonderful if our synthetic spectra could be processed in similar ways to true observed spectra, but it seems like the requirement of WCS in our spectra breaks this compatibility. Our synthetic spectra can currently be output as raw numpy arrays of wavelength, flux, and error fields, but I would be open to outputting them in an appropriate format for interoperation with astropy. I will follow the upcoming developments in astropy closely for updates.

[keflavich]

Hi @chummels,
This conversation is ongoing with this APE (astropy enhancement proposal): astropy/astropy-APEs#28 and an e-mail that I'll forward you.