Added a method to read an image into a pre-allocated array

[jishnub]

The read method currently used to allocate an array of the correct type and size and read from disk into it. This leads to temporary allocation that might hamper performance if data is read in a loop. In this PR a new method has been added to the non-allocating read! that lets us read from disk into a pre-allocated array.

[giordano]

Wow, this is an awesome first pull request, with documentation and a lot of tests! Thank you so much! This looks overall good to me, I only have two very minor comments, but I'd like to hear Kyle's comments, too 🙂

[giordano]

I think this refers to JuliaLang/julia#16717, right?

[jishnub]

Yes that's what I had in mind

[giordano]

Note that instead of using try/finally you can open the FITS file with

FITS(fname, "w") do f
    # ...
end

which will automatically close it at the end. You can find some examples in this file.

The try/finally that you still see here are leftovers of the times where this FITS method was not available, yet.

[kbarbary]

I think you still need this try...finally block to delete the temp file, no?

[jishnub]

I have updated the tests to use this method

[giordano]

@kbarbary there is rm(fname1, force=true) after the FITS ... do ... end block.

[jishnub]

The try finally might still be necessary to clean up if there's an error, in which case the rm won't get called. I have fixed this in the latest commit

[jishnub]

Additionally,

FITSIO.jl/src/libcfitsio.jl

Lines 799 to 801 in 450ddb6

	function fits_read_subset(
	f::FITSFile, fpixel::Vector{S1}, lpixel::Vector{S2},
	inc::Vector{S3}, data::Array{T}) where {S1<:Integer,S2<:Integer,S3<:Integer,T}

Does data need to be an array? I've tried redefining the method to match an AbstractArray, which lets me read data into views of an array as

julia> a=FITS("abc.fits","w"); write(a,ones(3,3)); close(a); a=FITS("abc.fits","r");

julia> b=zeros(3,3);

julia> read!(a[1],view(b,:,1),:,1)
3-element view(::Array{Float64,2}, :, 1) with eltype Float64:
 1.0
 1.0
 1.0

julia> b
3×3 Array{Float64,2}:
 1.0  0.0  0.0
 1.0  0.0  0.0
 1.0  0.0  0.0

Similarly in fits_read_pix. The tests seem to pass with this change.

[giordano]

Reading to a view is interesting, I happen to have this pending pull request for Julia to make Base.read! accept views: JuliaLang/julia#33046 (I should push it forward, by the way). AbstractArray is definitely too loose, even though we could just rely on missing methods to make it work, or you can look at the type I've used in my PR (Union{Array{T}, FastContiguousSubArray{T,N,<:Array{T}} where N}).

[giordano]

Yesterday I tested this PR locally and observed that read!(f1[1],b) does allocate something, even though the memory used is considerably less than read(f1[1]) and read! in the end is indeed slightly faster than plain read. I couldn't profile memory allocations, but they may well come from CFITSIO library.

[jishnub]

Reading to a view is interesting, I happen to have this pending pull request for Julia to make Base.read! accept views: JuliaLang/julia#33046 (I should push it forward, by the way). AbstractArray is definitely too loose, even though we could just rely on missing methods to make it work, or you can look at the type I've used in my PR (Union{Array{T}, FastContiguousSubArray{T,N,<:Array{T}} where N}).

Union{Array{T}, FastContiguousSubArray{T,N,<:Array{T}} where N} does seem to be the right way to implement this, I didn't know that FastContiguousSubArray exists.

[kbarbary]

Very nice PR! Just a couple minor comments.

[kbarbary]

I think you still need this try...finally block to delete the temp file, no?

[kbarbary]

These two lines should come first in the function, so that fits_get_img_equivtype operates on the intended extension

[jishnub]

I have fixed this in the latest commit.

[kbarbary]

Looks good to me!

[giordano]

Thank you so much!