Fix `with_spectral_unit` #1119

rosteen · 2024-01-22T23:49:55Z

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 · 2024-01-23T00:08:53Z

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.

dhomeier · 2024-01-23T14:24:58Z

specutils/utils/wcs_utils.py

+        return super().pixel_to_world(*args, **kwargs).to(
+            self.original_unit, equivalencies=u.spectral())
+
+


Perhaps just copy [pun-trigger!] over
https://github.com/astropy/astropy/blob/97bf1e123feec3170e2244964b0df2852b8e54e1/astropy/wcs/wcs.py#L633-L644
ff here as well for compatibility and to fix CI failure?

Weird, I thought I ran the tests locally after adding that, guess not. Thanks for the link.

Actually the tests do pass locally, I must have the IERS files cached.

Codestyle

rosteen · 2024-01-23T15:14:40Z

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 · 2024-01-23T15:33:47Z

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 · 2024-01-23T16:11:08Z

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

specutils/spectra/spectrum_mixin.py

Co-authored-by: Derek Homeier <[email protected]>

rosteen · 2024-01-23T18:20:37Z

@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 · 2024-01-23T18:57:33Z

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 · 2024-01-23T19:41:56Z

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.

dhomeier

Tried to set up an example of the deepcopy tests I commented on – setting Spectrum1D.meta itself is not foolproof though, so one might argue it is the user's responsibility not to mess with (shallow-)copied instances.

specutils/spectra/spectrum_mixin.py

specutils/tests/test_spectrum1d.py

Co-authored-by: Derek Homeier <[email protected]>

codecov · 2024-01-30T18:06:58Z

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.

❗ 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 ⚠️
specutils/utils/wcs_utils.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            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

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

rosteen · 2024-01-30T18:14:53Z

@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

Thanks; leaving some final remarks on the docs, but this looks good now!

specutils/spectra/spectrum_mixin.py

Co-authored-by: Derek Homeier <[email protected]>

dhomeier · 2024-02-01T18:34:01Z

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.

… references to SpectralRegion

dhomeier

Original problem solved, new barrel opened!
And just seeing uncertainty.py has an overwhich typo copied all over.
At least the docs are getting a good overhaul now. :D

specutils/analysis/moment.py

specutils/analysis/uncertainty.py

rosteen · 2024-02-01T20:40:41Z

@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 · 2024-02-01T20:52:05Z

@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 · 2024-02-01T21:00:03Z

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.

rosteen added 2 commits January 22, 2024 17:18

Fix with_spectral_axis when Spectrum1D instantiated with a spectral_axis

6439327

Deprecate old method name, new one is more specific

b8f439c

rosteen added the bug label Jan 22, 2024

rosteen added this to the v1.x milestone Jan 22, 2024

rosteen requested review from eteq, nmearl and keflavich as code owners January 22, 2024 23:49

Works for WCS-initialized spectra as well

f68ab6e

rosteen added 2 commits January 22, 2024 19:09

Remove unneeded imports

86f4744

Add kwarg default

88b2abd

rosteen requested a review from dhomeier January 23, 2024 00:12

dhomeier reviewed Jan 23, 2024

View reviewed changes

rosteen changed the title ~~Fix with_spectral_axis~~ Fix with_spectral_unit Jan 23, 2024

Add copy and deepcopy methods

0ff7bac

Codestyle

rosteen force-pushed the fix-with-spectral-unit branch from 5fb3f5d to 0ff7bac Compare January 23, 2024 14:50

dhomeier reviewed Jan 23, 2024

View reviewed changes

specutils/spectra/spectrum_mixin.py Outdated Show resolved Hide resolved

rosteen and others added 4 commits January 23, 2024 11:22

Update specutils/spectra/spectrum_mixin.py

5e9cc7f

Co-authored-by: Derek Homeier <[email protected]>

Add changelog entry

5221ce4

Test to make sure we kept the old WCS in meta

c54a30e

Better name for meta attribute, add test for it

c8159d2

dhomeier reviewed Jan 23, 2024

View reviewed changes

specutils/spectra/spectrum_mixin.py Outdated Show resolved Hide resolved

specutils/tests/test_spectrum1d.py Show resolved Hide resolved

rosteen and others added 2 commits January 30, 2024 12:59

Apply suggestions from code review

4949bea

Co-authored-by: Derek Homeier <[email protected]>

Fix codestyle

2a4f39a

dhomeier approved these changes Feb 1, 2024

View reviewed changes

specutils/spectra/spectrum_mixin.py Outdated Show resolved Hide resolved

Update docstring

eea7796

Co-authored-by: Derek Homeier <[email protected]>

rosteen added 2 commits February 1, 2024 15:16

Change docstring to fix docs build failure. Also fixed some incorrect…

217dbf0

… references to SpectralRegion

Fix one more SpectralRegion reference

2e357c8

dhomeier reviewed Feb 1, 2024

View reviewed changes

rosteen force-pushed the fix-with-spectral-unit branch from 90f92c6 to 2e357c8 Compare February 1, 2024 21:02

rosteen mentioned this pull request Feb 1, 2024

Fix broken SpectralRegion links in docstrings #1120

Open

rosteen merged commit 17a6f6b into astropy:main Feb 1, 2024
21 checks passed

This was referenced Feb 1, 2024

Spectrum1D.with_spectral_unit does not change spectral unit and overwrites WCS #889

Closed

What is the intended and actual result of spectrum.with_spectral_unit? #676

Closed

pllim mentioned this pull request Nov 15, 2024

Metadata Viewer: API to return dict, not list spacetelescope/jdaviz#3292

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `with_spectral_unit` #1119

Fix `with_spectral_unit` #1119

rosteen commented Jan 22, 2024

rosteen commented Jan 23, 2024

dhomeier Jan 23, 2024

rosteen Jan 23, 2024

rosteen Jan 23, 2024

rosteen commented Jan 23, 2024

dhomeier commented Jan 23, 2024

dhomeier commented Jan 23, 2024

rosteen commented Jan 23, 2024

dhomeier commented Jan 23, 2024 •

edited

Loading

rosteen commented Jan 23, 2024

dhomeier left a comment •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

rosteen commented Jan 30, 2024 •

edited

Loading

dhomeier left a comment

dhomeier commented Feb 1, 2024

dhomeier left a comment •

edited

Loading

rosteen commented Feb 1, 2024

rosteen commented Feb 1, 2024 •

edited

Loading

dhomeier commented Feb 1, 2024

		return super().pixel_to_world(args, *kwargs).to(
		self.original_unit, equivalencies=u.spectral())

Fix with_spectral_unit #1119

Fix with_spectral_unit #1119

Conversation

rosteen commented Jan 22, 2024

rosteen commented Jan 23, 2024

dhomeier Jan 23, 2024

Choose a reason for hiding this comment

rosteen Jan 23, 2024

Choose a reason for hiding this comment

rosteen Jan 23, 2024

Choose a reason for hiding this comment

rosteen commented Jan 23, 2024

dhomeier commented Jan 23, 2024

dhomeier commented Jan 23, 2024

rosteen commented Jan 23, 2024

dhomeier commented Jan 23, 2024 • edited Loading

rosteen commented Jan 23, 2024

dhomeier left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

rosteen commented Jan 30, 2024 • edited Loading

dhomeier left a comment

Choose a reason for hiding this comment

dhomeier commented Feb 1, 2024

dhomeier left a comment • edited Loading

Choose a reason for hiding this comment

rosteen commented Feb 1, 2024

rosteen commented Feb 1, 2024 • edited Loading

dhomeier commented Feb 1, 2024

Fix `with_spectral_unit` #1119

Fix `with_spectral_unit` #1119

dhomeier commented Jan 23, 2024 •

edited

Loading

dhomeier left a comment •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

rosteen commented Jan 30, 2024 •

edited

Loading

dhomeier left a comment •

edited

Loading

rosteen commented Feb 1, 2024 •

edited

Loading