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

Extend list of hacks for reading non-standard complied 1d WCS fits files #407

Merged
merged 3 commits into from
May 2, 2019

Conversation

hamogu
Copy link
Member

@hamogu hamogu commented Nov 28, 2018

In the current code, there is a hack to allow "LINEAR" as a spectral
axis. That's clearly not compliant with the FITS WCS standard as defined in
https://www.aanda.org/articles/aa/full/2006/05/aa3818-05/aa3818-05.html
and might just be motivated by the GMOS spectrum currently used as
example for this loader (@kelle ?).
This code change makes this list of keywords that will be treated as
spectral extensible and adds "Wavelength" (which is much closer to
"WAVE" as required in the standard than "LINEAR").

Tests and docs are still missing, but those are easy to add once we agree on this approach.

specutils/wcs/adapters/fitswcs_adapter.py Show resolved Hide resolved
specutils/wcs/adapters/fitswcs_adapter.py Outdated Show resolved Hide resolved
specutils/wcs/adapters/fitswcs_adapter.py Outdated Show resolved Hide resolved
@eteq
Copy link
Member

eteq commented Mar 8, 2019

For a quick example of a FITS header "in the wild" that this case might make work (note that as @nmearl pointed out this probably requires case-insensitive matching):

XTENSION= 'IMAGE   '           / Image extension                                
BITPIX  =                  -64 / array data type                                
NAXIS   =                    1 / number of array dimensions                     
NAXIS1  =                  940                                                  
PCOUNT  =                    0 / number of parameters                           
GCOUNT  =                    1 / number of groups                               
EXTNAME = 'DATA    '           / extension name                                 
BSCALE  =                  1.0                                                  
BZERO   =                  0.0                                                  
BUNIT   = ''                                                                    
WCSAXES =                    1 / Number of coordinate axes                      
CRPIX1  =                  1.0 / Pixel coordinate of reference point            
CDELT1  =                5E-09 / Coordinate increment at reference point        
CTYPE1  = 'WAVELENGTH'         / Coordinate type code                           
CRVAL1  =                6E-07 / Coordinate value at reference point            
LATPOLE =                 90.0 / [deg] Native latitude of celestial pole        

This might be a good unit test, actually? (i.e., load that as the WCS and give it a length-940 array as the flux)

@hamogu
Copy link
Member Author

hamogu commented Apr 24, 2019

@eteq: Given the major changes to all of this in #462 I'm very reluctant to add more tests and more files at this point. It will make that PR much more difficult to rebase. Also, I fail to run the tests on current master even with the most current released version of adsf and gwcs. I suspect that recent changes in specutils require the dev versions of adsf and gwcs. That makes development for an independent developer like me more difficult - I just want to update three lines of code here to read my fits file and not become a developer for all sorts of other packages that StScI is working on, too. Thus, I ask to merge this without new tests for now or to include this change in #462. Feel free to open an issue to add more tests and assign it to me. I'll do that once specutils works with the asdf and gwcs versions that I get from pypi again.

@nmearl
Copy link
Contributor

nmearl commented Apr 24, 2019

Specutils should work fine with the latest gwcs and asdf packages. I've created a new environment and ensured that all tests pass.

Just to verify: what errors are you seeing in your local test failures? What version of pytest and pytest-astropy do you have installed?

@hamogu
Copy link
Member Author

hamogu commented Apr 25, 2019

@nmearl I did some more debugging but still can't figure it out: #464

@hamogu
Copy link
Member Author

hamogu commented May 1, 2019

Resolved comments, added test (and updated one of the existing previous tests to actually test the warning message there, too), docs are in the class header. The loaders are not documented with a lot of text in the narrative docs; I feel that writing up a longer section on that is beyond the scope of this PR.

Ready for final review, if you ask me.

@hamogu
Copy link
Member Author

hamogu commented May 2, 2019

Test failures are all problems with the SDSS download. I've restarted the builds in question just so we get a nice looking green light but the SDSS test is not touched in this PR (and it passed locally a few hours ago).

Copy link
Contributor

@nmearl nmearl left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, but otherwise looks good!

specutils/wcs/adapters/fitswcs_adapter.py Outdated Show resolved Hide resolved
specutils/wcs/adapters/fitswcs_adapter.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
hamogu and others added 3 commits May 2, 2019 12:54
In the current code, there is a hack to allow "LINEAR" as a spectral
axis. That's clearly not complient with the FITS WCS standard as defined in
https://www.aanda.org/articles/aa/full/2006/05/aa3818-05/aa3818-05.html
and might just be motivated by the GMOS spectrum currently used as
example for this loader.
This code change makes ths list of keywords that will be treated as
spectral extensible and adds "Wavelength" (which is much closer to
"WAVE" as required in the standard than "LINEAR").
Co-Authored-By: hamogu <moritz.guenther@gmx.de>
@hamogu
Copy link
Member Author

hamogu commented May 2, 2019

@nmearl I accepted your suggestions and squashed the commits such that each commit includes one logical step.

@nmearl nmearl merged commit b873f2a into astropy:master May 2, 2019
@hamogu hamogu deleted the patchwcs branch May 2, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants