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?
| 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. |
There was a problem hiding this comment.
Thanks for the contribution. I updated one of the code comments based on @keflavich's suggestion, other than that this looks good.
|
Hmm, retracting my approval for now since tests are failing, I had forgotten or didn't realize that. @harry353 do you have time to address the test failures? From a quick glance my initial suspicion is that something was previously treating upper bounds as inclusive and is now treating them as exclusive. |
|
Yes, currently taking a look at this. |
harry353
force-pushed
the
extract-region-test-branch
branch
from
21a5e85 to
4cdb853
Compare
|
Fixed the off by one issue in region extraction (upper bound was including an extra bin). Now it lines up with normal Python slicing. Tests should all be passing now. |
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.