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

Fix SDSS-I/II spSpec units #1066

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Fix SDSS-I/II spSpec units #1066

merged 3 commits into from
Aug 2, 2023

Conversation

aragilar
Copy link
Contributor

It looks like something changed (I've not yet worked out what) in how astropy/wcslib handled absent CUNITa, and now the loader is incorrectly using metres instead of using angstroms. As the WCS was already incorrectly encoded, we probably can't trust CUNITa anyway, so force angstroms.

This also adds a test to confirm that the unit does not change again.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #1066 (38774cf) into main (a9c432c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1066   +/-   ##
=======================================
  Coverage   70.72%   70.72%           
=======================================
  Files          64       64           
  Lines        4485     4485           
=======================================
  Hits         3172     3172           
  Misses       1313     1313           
Files Changed Coverage Δ
specutils/io/default_loaders/sdss.py 47.19% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

"The WCS in the SDSS files does not appear to follow the WCS standard" o_o

Since it is wonky to begin with, I guess this is fine, but please confirm with SDSS that this is universal for all their DRs. If not, you might also need to filter by DR number or something?

@aragilar
Copy link
Contributor Author

aragilar commented Aug 1, 2023

The WCS issue is only for the spSpec files, so only affects SDSS-I and SDSS-II, SDSS-III and later use a different format and hence loader, and are based on FITS tables rather than WCS, and so avoid all the WCS unit issues.

https://classic.sdss.org/dr7/products/spectra/read_spSpec.php and https://classic.sdss.org/dr7/dm/flatFiles/spSpec.php document the format, and appear to be similar for the earlier data releases as well, and are identical for the relevant parts documenting the wavelength scale. These emphasise the misuse (i.e. sticking the log parameters into a linear WCS) is intentional, it may be that the format (or at least the choice of it) predates the standardisation of logarithmic WCS?

I've added links to the above pages to the docstrings, so the next person who looks at that code knows what the format is documented as (rather than having to dig through issues).

@pllim pllim requested a review from weaverba137 August 1, 2023 15:59
@pllim
Copy link
Member

pllim commented Aug 1, 2023

Hard to know if this works for sure given failure caused by this:

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

This all looks good to me. For historical perspective, the spSpec files were designed to be compatible with spectrum viewers in IRAF, so some differences in format are to be expected.

@pllim
Copy link
Member

pllim commented Aug 1, 2023

@aragilar , can you please rebase? The devdeps job should be all green unless this PR introduced new failures.

This should mean that future editors of those loaders (e.g. to add new
features or fix bugs) know what they can assume about the files.
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

CI is green. Thanks!

@pllim pllim merged commit f5967f5 into astropy:main Aug 2, 2023
10 checks passed
@pllim
Copy link
Member

pllim commented Aug 2, 2023

I dunno how this package do milestones, so I'll let @rosteen or @tepickering handle that.

@aragilar aragilar deleted the fix-sdss-units branch August 4, 2023 06:39
rosteen pushed a commit to rosteen/specutils that referenced this pull request Aug 15, 2023
* Add test for checking SDSS wavelength

* Enforce Angstroms in SDSS

* Add links to SDSS-I/II spectra formats docs

This should mean that future editors of those loaders (e.g. to add new
features or fix bugs) know what they can assume about the files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants