Allow spectral axis to be anywhere, instead of forcing it to be last #1033

rosteen · 2023-03-13T14:57:04Z

This replaces #999, merging into a new 2.0 development branch instead of main so that we can more thoroughly vet/test/discuss/etc big changes like this before releasing version 2.0. The context for the original PR is copied below, but please see #999 for previous discussion.

The motivation for this is that the insistence on the spectral axis being last hinders integration with other packages that make different (or no) assumptions on the location of the spectral axis. The two biggest examples, currently, are that spectral-cube assumes that the spectral axis is first, and they would be interested in using specutils objects as the basis of their code but are hindered by our assumption. We don't need to switch to that assumption, I think being flexible here is the right call. jdaviz also had to do a lot of coding around Glue assumptions about where the spectral axis is as well, since they differ from ours.

I have also been concerned for a while that specutils putting the flux axis into a different order than a user would get out from astropy.io.fits is potentially confusing and/or a barrier to uptake of specutils. In my mind at least, this combined with the above outweighs the convenience of numpy broadcasting properly automatically if you index solely on the spectral axis when it is last for a multi-dimensional Spectrum1D, which if I'm recalling correctly was the motivation for forcing the spectral axis to be last.

This does provide a move_spectral_axis keyword when initializing a Spectrum1D so that users can either get the legacy behavior (force it to be last) or do the opposite (force first) if desired.

keflavich

I have a few minor suggestions on implementation and UI.

This looks good!

docs/spectral_cube.rst

specutils/analysis/moment.py

specutils/spectra/spectrum1d.py

keflavich · 2023-03-13T16:13:34Z

specutils/spectra/spectrum1d.py

+                if move_spectral_axis is not None:
+                    if move_spectral_axis.lower() == 'first':
+                        move_to_index = 0
+                    elif move_spectral_axis.lower() == 'last':
+                        move_to_index = wcs.naxis - 1
+                    else:
+                        raise ValueError("move_spectral_axis must be either 'first' or 'last'")
+
+                    if move_to_index != self._spectral_axis_index:
+                        wcs = wcs.swapaxes(self._spectral_axis_index, move_to_index)
+                        if flux is not None:
+                            flux = np.swapaxes(flux, self._spectral_axis_index, move_to_index)
+                        if "mask" in kwargs:
+                            if kwargs["mask"] is not None:
+                                kwargs["mask"] = np.swapaxes(kwargs["mask"],


Just a note on this - I think it's fine as is, but we may need to add a convenience tool to reorder the celestial dimensions. If you give a cube with dims [RA, Dec, Spectral] and move_to_first now, you'll get [Spectral, Dec, RA], which is not what anyone would normally expect (imo). I don't think we should handle the logic here, but we might push a function in astropy.wcs that would rearrange_celestial_to_have_longitude_first

spectral-cube does this:
https://github.com/radio-astro-tools/spectral-cube/blob/master/spectral_cube/cube_utils.py#L204

Agreed - FYI this is currently what specutils does by default, so it's not any worse than it is currently 🙈 .

I added a note to the docs for now saying that RA/Dec might not be in a sensible order if you move the spectral axis.

keflavich · 2023-03-13T16:21:21Z

docs/spectrum1d.rst

+    >>> lower = [SkyCoord(ra=205, dec=26, unit=u.deg), SpectralCoord(4.9, unit=u.um)]
+    >>> upper = [SkyCoord(ra=205.5, dec=27.5, unit=u.deg), SpectralCoord(4.9, unit=u.um)]


Question: Can we give these slices in any order now? i.e., does specutils figure out which dimensions need to be sliced with which component? That would be ideal but I'm not sure if it's the case (yet)

Unfortunately no. I switched this here because the test was failing now that the spectral axis wasn't getting moved to last - the NDCube slicing machinery seems to expect the sky/spectral coords to be in the same order as in the WCS.

We should make an issue for this to make sure it's dealt with once this PR is merged!

I opened sunpy/ndcube#608.

codecov · 2023-03-13T19:54:55Z

Codecov Report

Attention: Patch coverage is 81.45161% with 23 lines in your changes missing coverage. Please review.

Project coverage is 70.57%. Comparing base (9422f9e) to head (338ce0f).
Report is 82 commits behind head on v2.0-dev.

Files with missing lines	Patch %	Lines
specutils/spectra/spectrum1d.py	77.65%	21 Missing ⚠️
specutils/analysis/moment.py	92.85%	1 Missing ⚠️
specutils/spectra/spectrum_mixin.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           v2.0-dev    #1033      +/-   ##
============================================
+ Coverage     70.25%   70.57%   +0.31%     
============================================
  Files            64       64              
  Lines          4418     4476      +58     
============================================
+ Hits           3104     3159      +55     
- Misses         1314     1317       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eteq

Broadly speaking... I think I'm convinced (good work on @keflavich and @Cadair wearing me down for years 😉 ). I do like the solution of still keeping last as an option but not forcing it.

There's two broad implementation concerns I want to raise though:

There's likely a lot of analysis/manipulation functions that still need to be made more flexible, right? E.g. the redshifting/template comparison stuff, and the resampling classes? That doesn't necessarily need to all be fixed now, although maybe it will be possible to use my next suggestion to do a "quick fix"
For those who have already built code assuming last-is-spectral, I wonder if it makes sense to also make a convenience accessor? Creating a new Spectrum1D with move_spectral_axis='last' is pretty syntactically annoying. But it would be pretty straightforward to add a get_last_spectral_axis_spectrum() method or something that just does that for you. Even better, it could cache the result (at least, optionally), which then would basically allow transparently using the "last" convention at least in cases where it's not computationally too intensive to do.

eteq · 2023-03-16T19:51:24Z

docs/spectrum1d.rst

+    >>> lower = [SkyCoord(ra=205, dec=26, unit=u.deg), SpectralCoord(4.9, unit=u.um)]
+    >>> upper = [SkyCoord(ra=205.5, dec=27.5, unit=u.deg), SpectralCoord(4.9, unit=u.um)]


We should make an issue for this to make sure it's dealt with once this PR is merged!

eteq · 2023-03-16T19:56:07Z

specutils/spectra/spectrum1d.py

+    move_spectral_axis : str, optional
+        Force the spectral axis to be either the last axis (the default behavior prior
+        to version 2.0) or the first axis by setting this argument to 'last' or 'first'.
+        This will do a simple ``swapaxis`` between the relevant axis and original
+        spectral axis.


Is there a reason this can't simply be an axis number? And then "last" can be -1, while "first" can be 0? Debatable if that's actually clearer than "last" vs "first", but I can see cases where one might want an in-between axis...

Also the None case means "don't change anything", right? That should be here, maybe something like:

Suggested change

move_spectral_axis : str, optional

Force the spectral axis to be either the last axis (the default behavior prior

to version 2.0) or the first axis by setting this argument to 'last' or 'first'.

This will do a simple ``swapaxis`` between the relevant axis and original

spectral axis.

move_spectral_axis : str, optional

Force the spectral axis to be either the last axis (the default behavior prior

to version 2.0) or the first axis by setting this argument to 'last' or 'first'.

This will do a simple ``swapaxis`` between the relevant axis and original

spectral axis. If None, the spectral axis is left wherever it is in the input.

I agree that this should let you input an integer, I'll make that change shortly. I may keep "first" and "last" as aliases for 0 and -1.

eteq · 2023-03-16T20:01:31Z

specutils/spectra/spectrum1d.py

+                    if len(matching_axes) == 1:
+                        self._spectral_axis_index = matching_axes[0]
+                    else:
+                        raise ValueError("Unable to determine which flux axis corresponds to "
+                                         "the spectral axis. Please specify spectral_axis_index"
+                                         " or provide a spectral_axis matching a flux axis.")


While I like the general idea of "best guess" here, there's one failure mode that worries me: the case where there's a multi-d flux axis and one of those axes just by chance happens to match the spectral axis. I can easily imagine someone writing code thinking the code just "knows" the correct axis, but then getting tripped up in a non-obvious way when there's an input that's ambiguous.

I'm not sure I have a clean solution though. A solution I don't love but might be good is to emit a warning whenever the guess is done, to hint to implementers they should really specify the axis if they can. But that will probably lead to a lot of annoying warnings.

Maybe @keflavich or @Cadair have some idea how common this failure mode might be? In optical-land I think it's rare enough it's not a big deal (since cubes are almost always a lot smaller than the spectral dimension) but I'm not as sure about other domains...

Isn't it required that at least one flux dimension matches the size of the spectral axis, if a spectral axis array is provided rather than a WCS? I'm not sure it makes any sense to create a Spectrum1D with flux shape, say, [10,10,200] and provide a spectral axis of length 240. This error would get thrown in that case, and it would also be raised if one of the spatial dimensions coincidentally happened to also match the length of the spectral axis.

Even in optical land, you'll work with data subsets, and these can have any shape.

If there's exactly one match, you're OK. If there are multiple matches or zero matches, error, right?

@rosteen What if you had x,y,spectral = [10,20,200] and a spectral axis was passed in with length 20? That would not result in this error but would be wrong. I'm .... not sure why a user would ever do this, though?

Hm, I see the point now. I'm also not sure why a user would do that, but then again...there have been many times when I wasn't sure why a user did something 😆. I guess the question is "is this likely enough to justify the annoyance of either displaying a warning, or always requiring spectral_axis_index?" And I'm not sure of the answer, although I lean toward leaving the code as-is and perhaps adding a more explicit/attention-grabbing warning in the documentation.

I'm loosely inclined toward no warning, but let's be prepared to post this conversation as a response to the first user error that results in this corner case.

rosteen · 2023-03-16T21:07:32Z

1. There's likely a lot of analysis/manipulation functions that still need to be made more flexible, right? E.g. the redshifting/template comparison stuff, and the resampling classes?  That doesn't necessarily need to all be fixed now, although _maybe_ it will be possible to use my next suggestion to do a "quick fix"

I did a pretty thorough search and replaced everything I could find that was assuming the spectral axis was last (mostly by looking for places that used -1 in an axis specification). To my admittedly non-professional-astronomer eye, it doesn't look like anything in redshifting and template comparison is assuming the spectral axis is last - is there something specific there that you think needs changing?

rosteen · 2023-03-22T18:37:07Z

@eteq @keflavich I believe that I addressed all of your comments, mind taking a look and making sure that I did approximately what you were suggesting?

keflavich

I have not done a thorough re-review, but you addressed my comments, afaict.

Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates

…mpatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example

Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment

Co-authored-by: Erik Tollerud <[email protected]>

Make this a docstring

rosteen · 2023-03-27T14:55:09Z

@eteq I believe I addressed all your points. I'm going to merge this, since it's going into the v2.0-dev branch and we have plenty of time to do additional work before version 2.0 will be released. I already have one follow-up issue I'm about to open, but I want to get this PR merged to close out this initial effort.

…stropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]>

* Allow spectral axis to be anywhere, instead of forcing it to be last (#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]> * Prepare changelog for 1.10.0 release * Fix Changelog * Fixed issues with ndcube 2.1 docs * Fix incorrect fluxes and uncertainties returned by FluxConservingResampler, increase computation speed (#1060) * new implementation of flux conserving resample * removed unused method * handle multi dimensional flux inputs * . * Update CHANGES.rst Co-authored-by: Erik Tollerud <[email protected]> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <[email protected]> * Update changelog for 1.11.0 release * Changelog back to unreleased * Working on retaining full GWCS information in Spectrum1D rather than just spectral coords * Handle getting the spectral axis out of a GWCS Add changelog heading Remove debugging prints Fix changelog Fix codestyle * Add changelog entry * Delete the commented-out old wavelength parsing code * More accurate changelog --------- Co-authored-by: Erik Tollerud <[email protected]> Co-authored-by: Nabil Freij <[email protected]> Co-authored-by: Clare Shanahan <[email protected]>

…stropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]>

* Allow spectral axis to be anywhere, instead of forcing it to be last (astropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]> * Prepare changelog for 1.10.0 release * Fix Changelog * Fixed issues with ndcube 2.1 docs * Fix incorrect fluxes and uncertainties returned by FluxConservingResampler, increase computation speed (astropy#1060) * new implementation of flux conserving resample * removed unused method * handle multi dimensional flux inputs * . * Update CHANGES.rst Co-authored-by: Erik Tollerud <[email protected]> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <[email protected]> * Update changelog for 1.11.0 release * Changelog back to unreleased * Working on retaining full GWCS information in Spectrum1D rather than just spectral coords * Handle getting the spectral axis out of a GWCS Add changelog heading Remove debugging prints Fix changelog Fix codestyle * Add changelog entry * Delete the commented-out old wavelength parsing code * More accurate changelog --------- Co-authored-by: Erik Tollerud <[email protected]> Co-authored-by: Nabil Freij <[email protected]> Co-authored-by: Clare Shanahan <[email protected]>

…stropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]>

* Allow spectral axis to be anywhere, instead of forcing it to be last (astropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]> * Prepare changelog for 1.10.0 release * Fix Changelog * Fixed issues with ndcube 2.1 docs * Fix incorrect fluxes and uncertainties returned by FluxConservingResampler, increase computation speed (astropy#1060) * new implementation of flux conserving resample * removed unused method * handle multi dimensional flux inputs * . * Update CHANGES.rst Co-authored-by: Erik Tollerud <[email protected]> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <[email protected]> * Update changelog for 1.11.0 release * Changelog back to unreleased * Working on retaining full GWCS information in Spectrum1D rather than just spectral coords * Handle getting the spectral axis out of a GWCS Add changelog heading Remove debugging prints Fix changelog Fix codestyle * Add changelog entry * Delete the commented-out old wavelength parsing code * More accurate changelog --------- Co-authored-by: Erik Tollerud <[email protected]> Co-authored-by: Nabil Freij <[email protected]> Co-authored-by: Clare Shanahan <[email protected]>

…stropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]>

* Allow spectral axis to be anywhere, instead of forcing it to be last (astropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]> * Prepare changelog for 1.10.0 release * Fix Changelog * Fixed issues with ndcube 2.1 docs * Fix incorrect fluxes and uncertainties returned by FluxConservingResampler, increase computation speed (astropy#1060) * new implementation of flux conserving resample * removed unused method * handle multi dimensional flux inputs * . * Update CHANGES.rst Co-authored-by: Erik Tollerud <[email protected]> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <[email protected]> * Update changelog for 1.11.0 release * Changelog back to unreleased * Working on retaining full GWCS information in Spectrum1D rather than just spectral coords * Handle getting the spectral axis out of a GWCS Add changelog heading Remove debugging prints Fix changelog Fix codestyle * Add changelog entry * Delete the commented-out old wavelength parsing code * More accurate changelog --------- Co-authored-by: Erik Tollerud <[email protected]> Co-authored-by: Nabil Freij <[email protected]> Co-authored-by: Clare Shanahan <[email protected]>

…stropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]>

* Allow spectral axis to be anywhere, instead of forcing it to be last (astropy#1033) * Starting to work on flexible spectral axis location Debugging initial spectrum creation Set private attribute here Working on debugging failing tests More things are temporarily broken, but I don't want to lose this work so I'm committing here Set spectral axis index to 0 if flux is None Working through test failures Fix codestyle Allow passing spectral_axis_index to wcs_fits loader Require specification of spectral_axis_index if WCS is 1D and flux is multi-D Decrement spectral_axis_index when slicing with integers Propagate spectral_axis_index through resampling Fix last test to account for spectral axis staying first Fix codestyle Specify spectral_axis_index in SDSS plate loader Greatly simply extract_bounding_spectral_region Account for variable spectral axis location in moment calculation, fix doc example Working on SpectrumCollection moment handling...not sure this is the way Need to add one to the axis index here Update narrative docs to reflect updates * Add back in the option to move the spectral axis to last, for back-compatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example * Apply suggestions from code review Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment * Address review comment about move_spectral_axis, more docs * Add suggested line to docstring Co-authored-by: Erik Tollerud <[email protected]> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <[email protected]> * Prepare changelog for 1.10.0 release * Fix Changelog * Fixed issues with ndcube 2.1 docs * Fix incorrect fluxes and uncertainties returned by FluxConservingResampler, increase computation speed (astropy#1060) * new implementation of flux conserving resample * removed unused method * handle multi dimensional flux inputs * . * Update CHANGES.rst Co-authored-by: Erik Tollerud <[email protected]> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <[email protected]> * Update changelog for 1.11.0 release * Changelog back to unreleased * Working on retaining full GWCS information in Spectrum1D rather than just spectral coords * Handle getting the spectral axis out of a GWCS Add changelog heading Remove debugging prints Fix changelog Fix codestyle * Add changelog entry * Delete the commented-out old wavelength parsing code * More accurate changelog --------- Co-authored-by: Erik Tollerud <[email protected]> Co-authored-by: Nabil Freij <[email protected]> Co-authored-by: Clare Shanahan <[email protected]>

rosteen mentioned this pull request Mar 13, 2023

Allow spectral axis to be anywhere, instead of forcing it to be last #999

Closed

rosteen requested review from nmearl, pllim, eteq and keflavich March 13, 2023 14:58

keflavich requested changes Mar 13, 2023

View reviewed changes

eteq requested changes Mar 16, 2023

View reviewed changes

rosteen force-pushed the free-the-axis branch from 3c7ba6b to e023bb1 Compare March 20, 2023 14:23

keflavich approved these changes Mar 22, 2023

View reviewed changes

rosteen and others added 6 commits March 27, 2023 10:39

Add back in the option to move the spectral axis to last, for back-co…

77c9ad8

…mpatibility Work around pixel unit slicing failure Change order on crop example Fix spectral slice handling in tuple input case (e.g. crop) Update output of crop example

Apply suggestions from code review

74d72d1

Co-authored-by: Adam Ginsburg <[email protected]> Apply suggestion from code review Add helpful comment

Address review comment about move_spectral_axis, more docs

82a618e

Add suggested line to docstring

df2f2c9

Co-authored-by: Erik Tollerud <[email protected]>

Add convenience method

87b5f27

Make this a docstring

rosteen force-pushed the free-the-axis branch from 2b9d626 to 87b5f27 Compare March 27, 2023 14:40

Add v2.0.0 changelog section

338ce0f

rosteen merged commit f4fc229 into astropy:v2.0-dev Mar 27, 2023

rosteen mentioned this pull request Mar 27, 2023

Make readers work with new move_spectral_axis keyword #1045

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow spectral axis to be anywhere, instead of forcing it to be last #1033

Allow spectral axis to be anywhere, instead of forcing it to be last #1033

rosteen commented Mar 13, 2023

keflavich left a comment

keflavich Mar 13, 2023

keflavich Mar 13, 2023

rosteen Mar 13, 2023

rosteen Mar 22, 2023

keflavich Mar 13, 2023

rosteen Mar 13, 2023

eteq Mar 16, 2023

rosteen Mar 22, 2023

codecov bot commented Mar 13, 2023 •

edited

Loading

eteq left a comment

eteq Mar 16, 2023

eteq Mar 16, 2023

rosteen Mar 20, 2023

eteq Mar 16, 2023

rosteen Mar 16, 2023 •

edited

Loading

keflavich Mar 16, 2023

rosteen Mar 16, 2023 •

edited

Loading

keflavich Mar 16, 2023

rosteen commented Mar 16, 2023

rosteen commented Mar 22, 2023

keflavich left a comment

rosteen commented Mar 27, 2023

		>>> lower = [SkyCoord(ra=205, dec=26, unit=u.deg), SpectralCoord(4.9, unit=u.um)]
		>>> upper = [SkyCoord(ra=205.5, dec=27.5, unit=u.deg), SpectralCoord(4.9, unit=u.um)]

Allow spectral axis to be anywhere, instead of forcing it to be last #1033

Allow spectral axis to be anywhere, instead of forcing it to be last #1033

Conversation

rosteen commented Mar 13, 2023

keflavich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2023 • edited Loading

Codecov Report

eteq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen commented Mar 16, 2023

rosteen commented Mar 22, 2023

keflavich left a comment

Choose a reason for hiding this comment

rosteen commented Mar 27, 2023

codecov bot commented Mar 13, 2023 •

edited

Loading

rosteen Mar 16, 2023 •

edited

Loading

rosteen Mar 16, 2023 •

edited

Loading