search

Starlink / ast

Starlink AST Library
GNU Lesser General Public License v3.0
12 stars 11 forks source link

Missing or incomplete support for FITS table column type 'K' - 64-bit integer #25

Closed gpdf closed 1 month ago

gpdf commented 1 month ago

SAOImageDS9/SAOImageDS9 is producing an error message

!! AST: Error in routine fits2TAB at line 3451 in file frame/fitsimage.C.
!  astFitsTable: Keyword 'TFORM1' in supplied FITS binary table header has unsupported value '7K'.

for an image file that, in addition to image extensions, also contains a binary table with 64-bit integer columns. Looking at astFitsTable I do see:

https://github.com/Starlink/ast/blob/b2fb006f4fb953607034dda5494fdb5bf114ab3f/src/fitstable.c#L978-L1002

where type 'K' is in fact not supported. Note that this type has been in FITS since version 2.1.

However, in keymap.h I do see

https://github.com/Starlink/ast/blob/b2fb006f4fb953607034dda5494fdb5bf114ab3f/src/keymap.h#L88

which appears to be an attempt to start supporting K. Further down in this file there are a number of routines supporting put/get for int64_t types.

I think probably the code in fitstable.c is just a bit behind and could now be updated to support 64-bit integers? It looks like K support would also have to be added to table.c as well, e.g., here:

https://github.com/Starlink/ast/blob/b2fb006f4fb953607034dda5494fdb5bf114ab3f/src/table.c#L2815-L2823

timj commented 1 month ago

It does look like this could be a pretty simple fix that even I might be able to manage. I'll give @dsberry a chance to comment.

dsberry commented 1 month ago

We're away in Crete at the moment. It would probably get to the end of September or start of October before I could look at it. Feel free to dive in yourself if you have the time.

dsberry commented 1 month ago

@timj It turned out to be a bit more complex than I expected because I needed to add K support to some other classes too. But I've pushed some commits to do this now.

gpdf commented 1 month ago

That's wonderful, thank you so much for the quick work! SPHEREx thanks you. 😄

timj commented 1 month ago

@dsberry Thank you for this. I think we can close this issue?

As a general point, since this added new APIs (without changing old one ones) it might have been better for the version number to be 9.3.0 rather than 9.2.11. The conda-forge packaging gets very concerned about ABI compatibility and using patch level versioning for changes causes confusion.

dsberry commented 1 month ago

ok