-
-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved GWCS handling in Spectrum1D #1074
Conversation
…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 <keflavich@gmail.com> 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 <erik.tollerud@gmail.com> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com>
…mpler, 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 <erik.tollerud@gmail.com> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com>
…just spectral coords
Add changelog heading Remove debugging prints Fix changelog Fix codestyle
7930be8
to
43cabcc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2.0-dev #1074 +/- ##
============================================
+ Coverage 71.03% 71.26% +0.23%
============================================
Files 64 64
Lines 4543 4542 -1
============================================
+ Hits 3227 3237 +10
+ Misses 1316 1305 -11 ☔ View full report in Codecov by Sentry. |
@@ -643,13 +644,10 @@ def _jwst_s3d_loader(filename, **kwargs): | |||
|
|||
# get mask information | |||
mask_name = primary_header.get("MASKEXT", "DQ") | |||
mask = hdulist[mask_name].data.T | |||
mask = hdulist[mask_name].data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, why was data being transposed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specutils currently demands that the spectral axis be last in a multidimensional spectrum. Specutils 2.0 is going to remove that requirements, so we'll be able to keep the data in the same shape as you would get if you read it in with say astropy.io.fits
.
|
||
1.11.0 (unreleased) | ||
|
||
1.12.0 (unreleased) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a change log entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just suggested a possible change to the change log entry to reflect the changes made here.
not essential as most lines are covered in this PR, but a test demonstrating creating a Spectrum1D with a GWCS might be appropriate too.
CHANGES.rst
Outdated
Other Changes and Additions | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
1.11.0 (unreleased) | ||
- JWST reader no longer transposes the input data cube for 3D data. [#1074] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if im wrong, this was just my understanding from our convo earlier - but would a better description be that Spectrum1D now allows GWCS as input, and when using the JWST reader to load a Spectrum1D the full GWCS is now loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that sound better?
I'm going to defer making a test/documentation for creating a Spectrum1D with user-created GWCS, I'll open an issue. |
* 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 <keflavich@gmail.com> 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 <erik.tollerud@gmail.com> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Clare Shanahan <cshanahan@stsci.edu>
* 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 <keflavich@gmail.com> 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 <erik.tollerud@gmail.com> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Clare Shanahan <cshanahan@stsci.edu>
* 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 <keflavich@gmail.com> 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 <erik.tollerud@gmail.com> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Clare Shanahan <cshanahan@stsci.edu>
* 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 <keflavich@gmail.com> 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 <erik.tollerud@gmail.com> * Add convenience method Make this a docstring * Add v2.0.0 changelog section --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> * omit removing units * added test to compare output to output from running SpectRes --------- Co-authored-by: Erik Tollerud <erik.tollerud@gmail.com> * 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 <erik.tollerud@gmail.com> Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Clare Shanahan <cshanahan@stsci.edu>
This PR allows Spectrum1D to keep the entire GWCS when reading a file (for example a JWST cube), rather than dropping any spatial WCS information as done previously. Probably most applicable beyond just JWST cubes but that's the use case I was targeting off the bat here. This has to go in v2.0 because GWCS doesn't have a
swapaxis
method like WCS does.