extract_region: preserve WCS for single regions, add velocity-region support #1263
Conversation
…region now preserves wcs info
| except Exception: | ||
| pass |
There was a problem hiding this comment.
It seems a little overly aggressive to catch all exceptions here. Can we limit this to UnitConversionError?
| @@ -128,6 +139,10 @@ def extract_region(spectrum, region, return_single_spectrum=False): | |||
| instead of multiple `~specutils.Spectrum` objects. The returned spectrum | |||
| will be a unique, concatenated, spectrum of all sub-regions. | |||
|
|
|||
| preserve_wcs: `bool` | |||
| If True, the WCS will be adjusted and retained in the output spectrum(s). | |||
| If False (default), WCS will be dropped. | |||
There was a problem hiding this comment.
The current False behavior does keep a WCS, it just turns it into a lookuptable WCS - so it doesn't exactly drop the WCS, it changes it.
| original_wcs = spectrum.wcs.deepcopy() | ||
|
|
||
| # Set CRPIX = 1.0 (FITS convention: reference pixel is 1-indexed) | ||
| original_wcs.wcs.crpix[0] = 1.0 | ||
|
|
||
| # Set CRVAL to match the first spectral axis value in the sliced spectrum | ||
| original_wcs.wcs.crval[0] = sliced.spectral_axis[0].to_value(original_wcs.wcs.cunit[0]) | ||
|
|
||
| sliced._wcs = original_wcs |
There was a problem hiding this comment.
maybe call this new_wcs instead, since you're modifying it? Functionally I think this looks fine, but I find the variable name a little confusing
There was a problem hiding this comment.
I see the intent, though - on the first line, you're copying the wcs from the unsliced spectrum, so there it's the original, but after you modify it, it's not.
…se now actually drops WCS entirely, rename WCS variable
|
Thanks for the quick feedback. Applied your suggestions in a new push. |
This update addresses issues #1261 and #1262, changing how the
extract_regionfunction behaves in two ways:preserve_wcs=True, the WCS is now kept for single-region extractions.It also includes a new test script to cover these cases.