Repository navigation
Implement SpectralCoord class #422
Description
As discussed with @eteq and @keflavich
In the APE 14 API, we currently use SkyCoord as the high-level object for celestial coordinates, but we only use Quantity for spectral coordinates, without any attributes that describe the frame of reference the spectral quantity is in, meaning that we can't easily convert between different spectral representations. The solution is to develop a SpectralQuantity that subclasses Quantity and adds metadata about the spectral frame as well as conversion functionality. This is best developed in specutils to start with for faster development, and we can incorporate it in to astropy core later.
SpectralQuantity should have the following attributes:
frame=: this would be aBaseFrameobjectrest=: a quantity giving the rest wavelength or frequency (assuming the same frame etc. as the parent quantity)type:radio_velocity,optical_velocity,frequency,energy,wavenumber,air_wavelength, etc.
In addition, we should make sure SpectralQuantity(SpectralQuantity(...), …) should work (needed for APE 14) and convert the spectral coordinate, as should e.g. SpectralQuantity.to(..., frame=’...’, type=’...’).
Any of the attributes can be passed into the constructor as None, in which case it basically just behaves like a normal Quantity. But if they are not None, to does smart things where possible and raises exceptions if anything would be inconsistent(i.e., to(u.um, type=’velocity’)).
We also decided that information about bin edges doesn’t belong inside this class - instead either there could be a SpectralAxis class that subclasses SpectralQuantity, or that contains e.g. a SpectralQuantity for the bin edges.
One item that still needs to be discussed is where to keep information about the direction of the observations - needed when transforming frames. One option is to include it as an attribute of the SpectalQuantity, and another option is to pass it as an argument during the transformation.
This should be developed in specutils in a file that should be well isolated from the rest of the code and could easily be moved to the core package once we are happy with it.
Activity
[-]Implement SpectralQuantity[/-][+]Implement SpectralQuantity class[/+]nmearl commentedon Dec 5, 2018
This is... interesting. So we want to move information about the axis to the axis's
Quantityrepresentation? Wouldn't this defeat the purpose of a mixin? Does this also presuppose that theQuantityhas information about the WCS to know which axis information is being used to generate theSpectralQuantitywhen used in parsing e.g. fits files? Is thetypefunctionally different from just a more fleshed outphysical_typethat already exists onQuantityobjects?astrofrog commentedon Dec 5, 2018
@nmearl - the primary idea of this class is that it is what will be returned by
wcs.pixel_to_worldwith the APE14 API. It doesn't replace the current mix-in class, but it means that this is the type of object that would end up being returned from e.g..spectral_axis. We should talk more in person tomorrow!nden commentedon Dec 5, 2018
Is there a reason why this (or part of it ) is not landing in GWCS? It seems a logical place.
stscicrawford commentedon Dec 5, 2018
As noted in the APE, this information should be part of the WCS:
https://github.com/astropy/astropy-APEs/blob/master/APE13.rst#wcs-spectrum1d-and-data-cubes
astrofrog commentedon Dec 5, 2018
We should discuss this in person :)
astrofrog commentedon Feb 8, 2019
To close the loop on this - we did speak about it in person at the coordination meeting with @crawfordsm, @nden, and @nmearl after this thread and there was general agreement that this should exist. One minor tweak was that @nmearl argued it should be called
SpectralCoord, which I agree with.[-]Implement SpectralQuantity class[/-][+]Implement SpectralCoord class[/+]nden commentedon Feb 8, 2019
I am also in favor of
SpectralCoord.We also talked about putting the initial version in gwcs to avoid circular dependencies.
nmearl commentedon Jul 12, 2019
Do we expect that conversions of the
SpectralCoordobject would live on theSpectralCoordor on the wcs? E.g.air_to_vacconversions create a new wavelength array, which I assume would mean that the wcs would need to update its lookup table to do pixel to world conversions.nden commentedon Jul 12, 2019
I'd expect something similar to
SkyCoord, i.e.SpectralCoordhas methods for converting the value from one "frame" to another, except in this case there might not be frames in the same sense as thecoordspackage. The WCS will outputSpectralCoordobjects which will know how to further transform.eteq commentedon Jul 26, 2019
@nmearl and I have been discussing out-of-band how the
SkyCoordmachinery translates over to this idea, and I'm copying some of the relevant discussion here so its publicly visible:I’m not sure it makes sense to use the
coordinatesmachinery directly in this context. It was designed (intentially) for 3D coordinates. I guess the question is: what of the machinery there is actually useful forSpectralCoord? Is it mainly the transform graph machinery, or the frame attribute machinery? Put another way: it may be more trouble than it’s worth to tie it directly into the coordinates frame/representation machinery, but one could imagine an independent thing that only uses the frame attribute machinery and none of the rest of it .From @nmearl: If we used the
SkyCoordas an analog, we might have aWavelengthRepresentationthat handles transforms to e.g.VelocityRepresentation/EnergyRepresentation/etc, and this would be attached to a frame object, but it’s not clear that if this frame object derives from the astropyBaseCoordinateFrameand given that its defaultly setup to deal in 3d space, it’d be able to understand how to transform a representation in 1d to a different coordinate frame.From me again: Another concern about
SpectralCoordbeing likeSkyCoord: if we wantSpectralCoordto be the thing that we get when we dospec.spectral_axis, that means it has to be different from thecoordinatesmachinery because it has to be like aQuantity. The Whereascoordinateframe classes contain things like aQuantity- you have to docoord.rainstead of justcoord, where the 1d nature ofSpectralCoordmakes that unnecessary.So the experiment to try would be to see if a light-weight
Quantitysubclass can be created that uses theFrameAttributeclasses and can be shoved into theFrameTransformGraphmachinery. If so then it’s a great re-use of the machinery, but I’m not sure if that’s trvial, reasonale but not trivial, or very hard.eteq commentedon Aug 9, 2019
There has been some substantial developments in the idea
SpectralCoordfollowing the python in astronomy conference. Suffice it to say it's a pretty complicated beast the more folks involved in that discussion thought about it. But some important realizations to summarize right now:SpectralCoordmight be fully described by aQuantitythat also has anobserver(which is anastropy.coordinateframe object, or could be None if unknown), atarget(similar toobserver), andrest(which is importantly not necessarily the same as "rv"/"redshift")redshift/rvinformation is encoded in the velocity offset between thetargetandobserver. We should have accessors/creatorsSpectralCoordshould be immutable like spatial coordinates, andto, etc yields newSpectralCoord'sTo try to progress, @nmearl, @astrofrog and I are trying to spec out some use cases to try to explore implementations. But that will probably be in astropy core since we now realize it needs to be in
wcsas well. Will need to figure out exactly how we want to end up using that here.eteq commentedon Feb 25, 2020
In a marathon session, @astrofog, @nmearl, and I worked out the standing issues we think still exist here. Some of these build from discussions in #524 rather than this issue itself, but I figured this is still the better place to put it:
From an earlier in-person discussion. Switch from the existing
to_rest/to_obsever/etc toin_velocity_frame(only adjusts velocity) andwith_observer(adjusts the observer position as well)Need to combine discussion threads in Implement SpectralCoord class #422, Implement spectral coordinate object #524, and Default observer and target for SpectralCoord #555
Do we still agree observer/target or is rest necessary?
observer/targetyes. But forrestwe realized we've mixed together "velocity" meaning the rest/vel conv/etc, and "redshift/rv" which are a very different use of velocity. We shouldn't do that. Below is a plan to separate them more cleanly.Eventually need to apply the barycentric correction way of doing RV (i.e., not a direct sum - see https://docs.astropy.org/en/stable/api/astropy.coordinates.SkyCoord.html#astropy.coordinates.SkyCoord.radial_velocity_correction for more). Not targeting that right now unless it comes "for free" with the coordinates machinery, but an eventual goal.
We will separate out the "apply the RV transformation" method from the "create new spectralcoord" operation
"with_observer" may actually have no real use. Is "in_velocity_frame" what we all care about? Decide to make
with_observerprivate (_change_observer_to, but make an issue about making it public if use case appears.Do we have any case where we want to shift "target"? No. In fact the only valid comparison for arbitrary observer/target combos is shifting to the velocity frame of the target. This should be a string shortcut for
in_velocity_frameof 'target' and/or 'rest' or possibly 'target-rest'? 'target' for sure, follow-on issue for 'rest'/etcWe should allow
target=Noneandobserver=Noneat class creation time. Then they can be set to value, but not re-setting once they've been set to something other than None.We know we want a "arbitrary rv shift" method. It should have the signature
with_los_shift(target=0, observer=0). Inputs can be either dimensionless, interpreted as redshift, velocity interpreted as rv, and all others raise exceptions. The specific choice of this signature is to allow for simple offsets of justwith_los_shift(20*u.km/u.s)for those who don't want to thing that hard, but more advanced control of the target/observer specifics for those who do care.restandvelocity_conventionin Implement spectral coordinate object #524 should be renameddoppler_restanddoppler_conventiondoppler_rest/doppler_conventionshould be liketarget/observerwhereNonemeans settable, but if already set, cannot be re-set. except by usingtoand explicitly setting therest/velocity_convention.In the end this leaves 4 attributes that are actual state of the
SpectralCoordin addition to the normalQuantitydata:target,observer(both SkyCoords),doppler_rest(velocity Quantity or None),doppler_convention(string or None)Revamp based on discussions in astropy#422
Revamp based on discussions in astropy#422
Revamp based on discussions in astropy/specutils#422
Revamp based on discussions in astropy/specutils#422
Revamp based on discussions in astropy/specutils#422