Skip to content
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

Refactor gwcs_from_array to provide ND GWCS in ND flux case #1211

Merged
merged 22 commits into from
Mar 18, 2025

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Feb 4, 2025

Currently, gwcs_from_array always gives a 1D purely spectral GWCS, even in the case where the flux array is multi-dimensional. This PR updates that function to return an ND GWCS, so that the dimensionality of a Spectrum's GWCS always matches that of the flux array, even if the spatial dimensions are simply returning the pixel values. This will simplify and solve a lot of problems down stream in e.g. glue-astronomy and jdaviz.

There were a lot of changes here that were needed to accommodate this change, but I think it's all improvements. I have one final failing test that I need to debug, so I'm opening this as a draft while I resolve that.

@rosteen rosteen added the data objects Core data objects like Spectrum1D or SpectralCollection label Feb 4, 2025
@rosteen rosteen added this to the v2.0 milestone Feb 4, 2025
@rosteen rosteen force-pushed the improve-gwcs-from-array branch from a9d0923 to 55cbb3e Compare February 7, 2025 19:49
@rosteen rosteen marked this pull request as ready for review February 9, 2025 23:25
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.94%. Comparing base (8403baf) to head (75aa0d7).
Report is 1 commits behind head on v2.0-dev.

Additional details and impacted files
@@             Coverage Diff              @@
##           v2.0-dev    #1211      +/-   ##
============================================
- Coverage     86.99%   86.94%   -0.06%     
============================================
  Files            63       63              
  Lines          4637     4687      +50     
============================================
+ Hits           4034     4075      +41     
- Misses          603      612       +9     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few minor comments but after taking a first look I realized one major concern/confusion so I'm going to start there and give another more thorough review depending on the outcome.

Specifically, it looks to me like the outcome of this change is essentially breaking the conceptual "link" between the spectral_axis and the wcs. What I mean is that in my head the way Spectrum1D is supposed to work is exactly as shown in this diagram:

https://specutils.readthedocs.io/en/latest/_images/specutils_classes_diagrams.png

in that diagram, the spectral_axis is basically just exactly what you'd get if you do spectrum.wcs.pixel_to_world(all_of_the_pixels_in_the_spectrum). (I know that's not literally how it's implemented but in my mind the API "contract" is that this is so) This would seem to break that link because now the dimensionality of wcs and spectral_axis are different. Is that the intent? If so, I have to ask: why not instead use SpectrumCollection for use cases where that's what's needed?

Comment on lines -409 to -411
(observer to target:
radial_velocity=0.0 km / s
redshift=0.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? Is it an incidental thing you noticed at the time (no prob if so) or a result of this change somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall, I'll double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was due to creating the new collapsed Spectrum with self.spectral_axis rather than self.wcs now, and is actually probably more correct than the previously printed out repr, which was replacing None values for these quantities with 0 in the process of collapsing (implying information we actually don't have).

Comment on lines +93 to +95
# We now raise an error if the spectra aren't on the same spectral axis
with pytest.raises(ValueError, match="Spectral axis of both operands must match"):
spec3 = simulated_spectra.s1_um_mJy_e1 + simulated_spectra.s1_AA_mJy_e3 # noqa
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this - I would have thought this failed previously - i.e. the shapes did not change. So is this also an incidental improvement that comes from these changes that's not strictly about the gwcs?

If so, that's good, but should be in the changelog as an API change I think (since users might encounter this as an error where they didn't before)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this should be in the changelog too. Previously spectrum arithmetic didn't care if the spectral axis values were different, only that the shape of the arrays were the same. It now checks to see that the values actually match as well.

@rosteen
Copy link
Contributor Author

rosteen commented Feb 28, 2025

in that diagram, the spectral_axis is basically just exactly what you'd get if you do spectrum.wcs.pixel_to_world(all_of_the_pixels_in_the_spectrum). (I know that's not literally how it's implemented but in my mind the API "contract" is that this is so) This would seem to break that link because now the dimensionality of wcs and spectral_axis are different. Is that the intent? If so, I have to ask: why not instead use SpectrumCollection for use cases where that's what's needed?

This is already not the case for things like JWST cubes, which provide a full 3D GWCS solution that encodes both the spectral axis and spatial coordinates. The issue is that specutils is inconsistent in whether a multidimensional spectral cube can be expected to have a WCS/GWCS of matching dimensionality with the flux array (when you're reading a FITS file, generally) or a 1D GWCS (the case where you're manually creating a Spectrum1D with just a spectral_axis specified). The latter case causes all sorts of problems with downstream packages that expect the dimensionality of a WCS/GWCS to match the dimensionality of the data - for example there is an ugly work-around in glue-astronomy to pad out the WCS in this case with dummy dimensions (similar to what I'm implementing here) so that Glue doesn't choke on the dimensionality mismatch when we load something further downstream in Jdaviz.

My goal here is to bring the case where we generate a dummy GWCS for a spectrum created with a spectral_axis rather than wcs specified in line with the (reasonable, IMO) expectation that a multidimensional flux array would have a multidimensional WCS/GWCS. This implementation does that by just passing pixel values through as the spatial coordinates, which should allow a lot of code simplification downstream in a couple packages. This does mean that the diagram you referenced isn't accurate and should be updated, but as I said, it already wasn't really accurate for any multidimensional Spectrum1D created by reading a FITS file, where the dimensionality of the WCS already matches the flux array, not the spectral axis.

@rosteen
Copy link
Contributor Author

rosteen commented Mar 18, 2025

@eteq I'm going to go ahead and merge this into the v2.0-dev branch so I can release an RC for people to start testing 2.0. I'll start a discussion issue where people can post concerns about 2.0 to address before an actual release, we can continue the discussion of this here or take it to that issue if my replies didn't alleviate your concerns.

@rosteen rosteen merged commit 7c3a160 into astropy:v2.0-dev Mar 18, 2025
12 of 13 checks passed
rosteen added a commit to rosteen/specutils that referenced this pull request Mar 18, 2025
…1211)

* Working on constructing N dimensional GWCS for ND flux with only spectral axis provided

* Get forward transform working for ND case

* Convert other operator to Spectrum if needed in specutils where we have more control, rather than in astropy ndarithmetic

* Don't pass extra arg to ndarithmetic

* Use dimensionless instead of pix, remove unit conversion code that I think was unneeded and was causing errors

* Fix two tests for multidimensional GWCS, remove debugging print

* Working on debugging SpectrumCollection

* Check for Spectrum specifically before arithmetic

* Remove incorrect index from spectrum collection slice, cast operand as Spectrum even in 1D case

* Don't pass multi-d WCS to collapsed spectrum

* Fixing more test failures

* Revamp arithmetic

* Add changelog

* remove debugging print

* Handle creating dummy WCS and spectral axis when neither are provided, fix order of WCS

* Codestyle

* Remove debugging prints

* Fixes for compatibility with up to date gwcs

* Bump min version pins

Bump min asdf

Bump min astropy

Bump min scipy

Bump min asdf-astropy

* Need gwcs 0.24

* Add note about arithmetic to changelog

* Fix Spectrum1D references in doc page
rosteen added a commit to rosteen/specutils that referenced this pull request Mar 18, 2025
…1211)

* Working on constructing N dimensional GWCS for ND flux with only spectral axis provided

* Get forward transform working for ND case

* Convert other operator to Spectrum if needed in specutils where we have more control, rather than in astropy ndarithmetic

* Don't pass extra arg to ndarithmetic

* Use dimensionless instead of pix, remove unit conversion code that I think was unneeded and was causing errors

* Fix two tests for multidimensional GWCS, remove debugging print

* Working on debugging SpectrumCollection

* Check for Spectrum specifically before arithmetic

* Remove incorrect index from spectrum collection slice, cast operand as Spectrum even in 1D case

* Don't pass multi-d WCS to collapsed spectrum

* Fixing more test failures

* Revamp arithmetic

* Add changelog

* remove debugging print

* Handle creating dummy WCS and spectral axis when neither are provided, fix order of WCS

* Codestyle

* Remove debugging prints

* Fixes for compatibility with up to date gwcs

* Bump min version pins

Bump min asdf

Bump min astropy

Bump min scipy

Bump min asdf-astropy

* Need gwcs 0.24

* Add note about arithmetic to changelog

* Fix Spectrum1D references in doc page

Fix skips in doc file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data objects Core data objects like Spectrum1D or SpectralCollection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants