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

Make HDU types parametric #119

Closed giordano closed 4 years ago

giordano commented 4 years ago

Would it make sense to make the HDU concrete types parametric? E.g., ImageHDU{T, N}, with T the data type and N the number of dimensions of the underlying data array. This should also make read(...) type stable.

Ideally we could do something similar for the tables, but it's less clear how to encode the information, as they depend also on the specific column

emmt commented 4 years ago

Yes I think that it would be a very good idea. Type stability is a very good point. Parameters {UInt8,0} could be used in case there is no data. One drawback is that you must known in advance the element type and the number of dimensions in advance whene creating an Image HDU.

Owing to the variety of the possible column types in FITS Tables, I don't think this could be done for tables.

giordano commented 4 years ago

One drawback is that you must known in advance the element type and the number of dimensions in advance whene creating an Image HDU.

We know this information. The type of the array returned when reading the ImageHDU is TYPE_FROM_BITPIX[fits_get_img_equivtype(hdu.fitsfile)], and the number of dimensions is length(fits_get_img_size(hdu.fitsfile)).

emmt commented 4 years ago

OK this is when reading data but you may want to create a FITS Image by concatenating data so that you only know the result at the end and update the FITS header accordingly. My point was that, with your suggested change, this is possible providing you know the number of dimensions in advance (not their lengths) which I think is fairly resonnable and is not such a big restriction. So I am totally in favor of your suggestion.

kbarbary commented 4 years ago

I don't think type stability is so important here, at least when reading. My reasoning is that getindex(::FITS, ...) is not type stable, so this change just moves type instability to a slightly different place.

I'm away from my laptop at the moment, but I'll follow up with a bit more in #120 when I'm back, probably tomorrow.

giordano commented 4 years ago

I'm closing this, as per discussion in #120