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 misinterpretation of byte order of blocks stored in FITS files #810

Merged

Conversation

eslavich
Copy link
Contributor

This PR fixes an issue with reading binary blocks stored in FITS files.

When astropy reads data that requires scaling, it performs said scaling and returns the result in native byte order. Until now asdf has been assuming that all data from a FITS file is big-endian, since that's how the bytes are stored on disk, but due to the scaling operation that's not always the case.

The fix is to accept that the dtype reported by an array from a FITS file is correct.

CC @stscieisenhamer

@eslavich eslavich added the bug label Jun 12, 2020
@eslavich eslavich added this to the 2.6.1 milestone Jun 12, 2020
@eslavich eslavich requested a review from nden June 12, 2020 19:48
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #810 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   94.26%   94.27%   +0.01%     
==========================================
  Files          42       42              
  Lines        4971     4980       +9     
==========================================
+ Hits         4686     4695       +9     
  Misses        285      285              
Impacted Files Coverage Δ
asdf/block.py 96.46% <100.00%> (+0.01%) ⬆️
asdf/fits_embed.py 92.80% <100.00%> (+0.15%) ⬆️
asdf/tags/core/ndarray.py 94.00% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad141da...24f969d. Read the comment docs.

@stscieisenhamer
Copy link

FYI: I tested on the case that prompted the issue report and it does fix the issue.

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

Well, this gives me a chance to review a part of the code I've never looked over before. Using this trickery of ASDF redirecting its blocks to FITS extension data sections makes me a bit queasy, not that I have better suggestions (though I wonder if making the FITS references more explicit and requiring manual data extraction might have been a better thing; but I haven't really thought about it).

I wonder why it took this long to find the problem. Seems like any access of raw data would have triggered this. I'll ask Jonathan.

@perrygreenfield perrygreenfield merged commit 453d7a7 into asdf-format:master Jun 16, 2020
@jdavies-st
Copy link
Contributor

I suspect this was hidden because jwst is the only package that writes ASDF to FITS files, and there was a bug in jwst.datamodels layered on top of it that also munged the unsigned ints that @eslavich fixed a year ago.

spacetelescope/jwst#3680

@perrygreenfield
Copy link
Contributor

Yeah but it has been doing that for a while. Did the jwst.datamodels bug actually counteract the ASDF bug? (so two wrongs do make a right?)

@eslavich eslavich deleted the eslavich-fix-fits-byte-order branch June 16, 2020 20:33
@eslavich eslavich modified the milestones: 2.6.1, 2.7.0 Jul 16, 2020
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.

4 participants