Fix `with_spectral_unit`

[rosteen]

This has been needed for a long time - intended to fix #995 , #889 , #676. My current changes fix the behavior for a Spectrum1D initialized with a spectral_axis, but I think it needs a little more work to also fix it for one initialized with a wcs. Note that this also moves the SpectralGWCS out of gwcs_from_array so that it's actually accessible to check instances, since there are times we may want to do something different if that's the WCS type.

[rosteen]

Ok, I realized the existing WCS-initialized spectrum case was just acting on the spectral_axis as well, so I didn't need the a special case for the spectral_axis-initialized case and handle both the same way. It would probably be more ideal to preserve as much of the original WCS as possible while changing the unit, but that can be work for another day. This at least restores the previous (intended) functionality.

[rosteen]

Looks like the remaining failures are a (hopefully transient) download failure:

astropy.utils.iers.iers.IERSWarning: failed to download https://datacenter.iers.org/data/9/finals2000A.all: <urlopen error [Errno -3] Temporary failure in name resolution>

[dhomeier]

Yes, seen those frequently, although they did not show up in the previous run; unlikely this was introduced by adding the copy methods. Can maybe check if astropy has done something to avoid these connection failures.

[dhomeier]

Seems the last solution was just to wait till DNS is working again... astropy/astropy#15456

[rosteen]

@dhomeier I switched to deepcopy and added a test to check that the stored copy of the original WCS has the original unit. If this looks good to you now would you approve so I can merge? Thanks!

[dhomeier]

The test I did locally to verify it really needs deepcopy was (kinda brute force) setting new_spec.wcs.cdelt (with an Astropy WCS) to different values and check if the original spec.wcs.cdelt was changed by this. But SpectralGWCS might not be modified that easily, so maybe that's overkill. Just noted that the entire meta is copied as well; being a dictionary, this would probably need a deepcopy as well to ensure you have separate objects.

[rosteen]

check if the original spec.wcs.cdelt was changed by this

Doesn't checking that the world_axis_units is different between the original WCS and new WCS confirm that they are different objects? I think this test is accomplishing the same thing as yours.

Just noted that the entire meta is copied as well; being a dictionary, this would probably need a deepcopy as well to ensure you have separate objects.

The meta is an OrderedDict, which doesn't seem to have deepcopy. I just confirmed that meta is self._meta returns False after the copy, so I think this is ok.

[codecov[bot]]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4723206) 70.89% compared to head (90f92c6) 70.93%. Report is 2 commits behind head on main.

:exclamation: Current head 90f92c6 differs from pull request most recent head 2e357c8. Consider uploading reports for the commit 2e357c8 to get more accurate results

Files	Patch %	Lines
specutils/spectra/spectrum_mixin.py	92.85%	1 Missing :warning:
specutils/utils/wcs_utils.py	92.30%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1119 +/- ## ========================================== + Coverage 70.89% 70.93% +0.03% ========================================== Files 64 61 -3 Lines 4501 4221 -280 ========================================== - Hits 3191 2994 -197 + Misses 1310 1227 -83 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[rosteen]

@dhomeier I did update the docstring already to explain what happens to the original WCS, is that enough to address the concern you brought up about documentation earlier today offline?

[dhomeier]

Not sure – do we get

WARNING: py:class reference target not found: specutils.utils.wcs_utils.SpectralGWCS

because that version is not in the online docs yet, or actually because SpectralGWCS does not have a docstring yet?

But I am seeing other broken links into utils that apparently have not broken the build.

[rosteen]

@dhomeier After some tinkering I couldn't get that doc link to work (I was breaking more things...) so I decided to just make the docstring a little more descriptive here for now as a compromise. I pointed those other utils doc links to the proper place, but they still seem to be broken rather than linking 🤔. I'll come back to that at some point but I have more pressing concerns at the moment.

[rosteen]

@dhomeier As I commented above on one of your suggestions, you can see here that using the full path to SpectralRegion doesn't fix the broken links. If you want to debug that and open a follow-up PR that would be awesome, but I don't think it should hold up this one.

[dhomeier]

Sorry, was a bit too fast in committing then – maybe you should force-push again to revert. Sphinx keeps puzzling me; I just noted that in the docs themselves the recommended path is

from specutils.spectra import SpectralRegion

but this can wait for another PR as well.