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

Invalid bitpix value from ffgiet causing panic #35

Closed
saethlin opened this issue Jul 11, 2017 · 7 comments
Closed

Invalid bitpix value from ffgiet causing panic #35

saethlin opened this issue Jul 11, 2017 · 7 comments

Comments

@saethlin
Copy link

rust-fitsio uses ffgiet to determine the dtype of an image HDU. I have an image for which ffgiet returns 20, which runs me into an unreachable!(). The cfitsio docs suggest this shouldn't be possible. The header starts off like this:

SIMPLE  =                    T                                                  
BITPIX  =                   16 /8 unsigned int, 16 & 32 int, -32 & -64 real     
NAXIS   =                    2 /number of axes                                  
NAXIS1  =                 1024 /fastest changing axis                           
NAXIS2  =                 1024 /next to fastest changing axis                   
BSCALE  =   1.0000000000000000 /physical = BZERO + BSCALE*array_value           
BZERO   =   32768.000000000000 /physical = BZERO + BSCALE*array_value           

Here's a link to download the full file

As a workaround for this one file, I can use ffgidt. Is this a bug in cfitsio?

@saethlin saethlin changed the title Invalid bitpix value from ffgiet Invalid bitpix value from ffgiet causing panic Jul 11, 2017
@simonrw
Copy link
Owner

simonrw commented Jul 11, 2017

Hi @saethlin, it looks like a value of 20 is unsigned short which must be being interpreted through the combination of BSCALE and BZERO. I have been pondering the use of ffgiet vs ffgidt for a little while, and perhaps need to switch to this function. I have not got time to look into it now, but hopefully a little later. thanks for reporting!

@simonrw
Copy link
Owner

simonrw commented Jul 11, 2017

So after playing around with this, there are two ways to solve the problem:

  1. change ffgiet to ffgidt
  2. include 20 => ImageType::USHORT_IMG in the list (types.rs:57)

The first solution changes from fits_get_img_equivtype to fits_get_img_type which seems better as it's first in the documentation, and generally that means the function should be preferred, but the description makes it sound like the second function is more true to what actually happens (e.g. with integer data and floating point bscale the type returned by fits_get_img_equivtype is a floating point type).

In addition, another cfitsio wrapper library fitsio uses fits_get_img_equivtype (see fitsio_pywrap.c:631).

Based on a very unscientific test, searching fits_get_img_equivtype in github gives 482 results, whereas fits_get_img_type gives >1k results.

@saethlin do you have any insights?

@saethlin
Copy link
Author

Mostly I'm bothered that cfitsio provides two ways to do the same task, but in general I lean towards fits_get_img_type because it's more explicit and doesn't return bitpix values that aren't valid for the BITPIX header field. Even if it's not what the cfitsio authors may hope for, I think fgidt is more maintainable.

@simonrw
Copy link
Owner

simonrw commented Jul 11, 2017

I agree, the inconsistency in cfitsio is annoying, but we can help improve things by making the interface more intuitive. If the image data is floating point, stored as integers with BSCALE/BZERO appropriately, then the image data type presented to the user should be floating point. We can achieve this with fits_get_img_equivtype. It also saves this crate from doing extra BSCALE/BZERO checking when we could just let cfitsio do it.

@simonrw simonrw reopened this Jul 11, 2017
@simonrw
Copy link
Owner

simonrw commented Jul 13, 2017

@saethlin how do you feel about the changes I've made? Now USHORT_IMG data types are supported, and do not error, and I still feel fits_get_img_equivtype is my preferred choice.

@simonrw
Copy link
Owner

simonrw commented Oct 9, 2017

@saethlin have you had a chance to test the changes I've made?

@simonrw
Copy link
Owner

simonrw commented Oct 25, 2017

My tests show this is not an issue any more, and your example file reads correctly. I'm closing this issue now. Thanks for your help and guidance!

@simonrw simonrw closed this as completed Oct 25, 2017
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

No branches or pull requests

2 participants