-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1066 +/- ##
=======================================
Coverage 70.72% 70.72%
=======================================
Files 64 64
Lines 4485 4485
=======================================
Hits 3172 3172
Misses 1313 1313 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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?
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). |
Hard to know if this works for sure given failure caused by this: |
There was a problem hiding this 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.
@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.
61ede4e
to
38774cf
Compare
There was a problem hiding this 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!
I dunno how this package do milestones, so I'll let @rosteen or @tepickering handle that. |
* 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.
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 trustCUNITa
anyway, so force angstroms.This also adds a test to confirm that the unit does not change again.