Adding redshift and radial velocity properties to SpectrumCollection#1332
Adding redshift and radial velocity properties to SpectrumCollection#1332rosteen wants to merge 6 commits into
Conversation
…allows only scalar values
rosteen
added
the
data objects
keflavich
left a comment
keflavich
left a comment
There was a problem hiding this comment.
I have some minor wording suggestions.
I'm not sure it is ever right to adopt the first redshift out of a collection if they're not all equal
| if redshift is not None and not (isinstance(redshift, (int, float)) or | ||
| (isinstance(redshift, u.Quantity) and redshift.ndim == 0)): | ||
| raise ValueError("Only single-value redshifts are supported at this time.") |
There was a problem hiding this comment.
maybe np.isscalar can replace this logic?
There was a problem hiding this comment.
Unfortunately it doesn't work on Quantities, so it can't replace everything here.
| redshift = spectra[0].redshift | ||
| if (not all((x.redshift == redshift for x in spectra))): | ||
| warnings.warn("Not all redshifts are equal, using redshift of first spectrum.") |
There was a problem hiding this comment.
maybe this should raise an exception? I'm not sure when using the first would be acceptable
There was a problem hiding this comment.
I was iffy on this, I'll change it to an exception.
There was a problem hiding this comment.
Oh, turns out this already would have been caught by an earlier check of radial_velocity (which raises an exception), I removed this.
Co-authored-by: Adam Ginsburg <keflavich@gmail.com>
2 participants
Currently only allow scalar values (rather than an array of values matching the number of spectra), and allows shifting the spectral axes with
shift_spectrum_tolikeSpectrumdoes. I also added some clarification to the narrative docs about what exactly is happening when you access a single spectrum from a SpectrumCollection. Allowing array redshifts (one per spectrum in the collection) requires some upstream work in Astropy first.Closes #1318.