JuliaAstro / FITSIO.jl

Flexible Image Transport System (FITS) file support for Julia
http://juliaastro.org/FITSIO.jl/
MIT License
55 stars 29 forks source link

Can the signature of `Libcfitsio.fits_write_key` be widened? #61

Closed mweastwood closed 7 years ago

mweastwood commented 7 years ago

I have a TLONG that I would like to write to the header using Libcfitsio.fits_write_key (the function is mirroring a C routine so I want to use the Libcfitsio interface). At the moment the function only accepts floats and strings (see the link below), but it naively looks like it might have been written with a wider signature in mind.

https://github.com/JuliaAstro/FITSIO.jl/blob/8f5383601c530dcfbce1b9502f82f8f4bcdb54e7/src/libcfitsio.jl#L347-L358

Edit: grammar and typos

giordano commented 7 years ago

Out of curiosity, what's a TLONG type? How would you suggest to change the signature of the function?

mweastwood commented 7 years ago

TLONG is just a Clong in Julia (can be a 32-bit integer or a 64-bit integer depending on the system). So I'd like fits_write_key to at least work with integer value as well.

It looks like the function was written under the assumption that value might be a Bool, but this is prohibited by the function's signature. So I am hoping that the function might still work as written even after simply widening the signature to accept Bool, Int32, Int64, etc.

giordano commented 7 years ago

Thanks! I found out the meaning of TLONG in the comment, and right now I was looking to the same inconsistency: a Bool value is allowed in the definition of cvalue, but value can't be a Bool in the signature of the function. In other places (like fits_write_pix, fits_read_pix, fits_read_subset, fits_read_col, and fits_write_col), a generic T type is used, without further specification, probably the same can be done here.

kbarbary commented 7 years ago

I found the culprit (me in 2014): https://github.com/JuliaAstro/FITSIO.jl/commit/4eb54856a4086cd361e157ca28f72ba3067e1838#diff-7f30490ba5728cf8a2e44da573ed477dL212

Yes, this should definitely be widened! Not sure what happened there.