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 ImageHDU parametric #158

Closed jishnub closed 3 years ago

jishnub commented 3 years ago

This helps make the size and read functions type-stable.

After this PR:

julia> fname = tempname() * ".fits"
"/tmp/jl_sFQ6s6.fits"

julia> FITS(fname, "w") do f
           write(f, ones(2,2))
       end

julia> f = FITS(fname);

julia> @code_warntype read(f[1])
Variables
[...]

Body::Array{Float64,2}
[...]

julia> @code_warntype size(f[1])
Variables
[...]

Body::Tuple{Int64,Int64}
[...]

As a consequence: on master:

julia> FITS(fname, "w") do f
       write(f, ones(100, 100))
       end

julia> f = FITS(fname,"r");

julia> @btime read($(f[1]));
  39.639 μs (12 allocations: 78.89 KiB)

After this PR:

julia> @btime read($(f[1]));
  23.956 μs (5 allocations: 78.48 KiB)

The time difference is not significant for large arrays though when reading from disk dominates the execution time.

  1. this PR adds a feature to read! to read into an array with a different shape, as long as the number of elements match that of the image section to be read in. The signature of read! is made a bit stricter, although this is non-breaking (the change is in the type of the error thrown if the eltypes don't match).
    
    julia> FITS(fname, "w") do f
       write(f, ones(2,2))
       end

julia> b = zeros(4);

julia> read!(f[1], b);

julia> b 4-element Array{Float64,1}: 1.0 1.0 1.0 1.0


3. Check if file is opened in read-only mode before writing to it. This used to segfault on master, fixed now. After this PR:
```julia
julia> f = FITS(fname, "r");

julia> write(f, ones(3,3))
ERROR: ArgumentError: FITS file has been opened in read-only mode
  1. Adds Aqua.jl as a test dependency to help maintain project quality.
codecov[bot] commented 3 years ago

Codecov Report

Merging #158 (207fd27) into master (e69d9b8) will increase coverage by 0.75%. The diff coverage is 89.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   88.80%   89.55%   +0.75%     
==========================================
  Files           5        5              
  Lines         625      613      -12     
==========================================
- Hits          555      549       -6     
+ Misses         70       64       -6     
Impacted Files Coverage Δ
src/image.jl 93.38% <88.37%> (+3.18%) :arrow_up:
src/FITSIO.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e69d9b8...207fd27. Read the comment docs.

giordano commented 3 years ago

See #120

jishnub commented 3 years ago

Good points, I had missed that. Evidently the only difference in read is a reduction in allocations, but that hardly impacts performance. There are marginal performance improvements in size and eltype and such.

On master:

julia> fname = tempname() * ".fits";

julia> FITS(fname, "w") do f
       write(f, ones(100, 100))
       end

julia> f = FITS(fname);

julia> sizehdu1(f) = size(f[1]);

julia> eltypehdu1(f) = eltype(f[1]);

julia> @btime sizehdu1($f)
  417.829 ns (2 allocations: 128 bytes)
(100, 100)

julia> @btime eltypehdu1($f)
  7.731 μs (0 allocations: 0 bytes)
Float64

After this PR:

julia> @btime sizehdu1($f)
  159.929 ns (2 allocations: 128 bytes)
(100, 100)

julia> @btime eltypehdu1($f)
  44.430 ns (0 allocations: 0 bytes)
Float64

If a typed ImageHDU is not something worth adding to this package, I can remove that and retain the other features in this PR.

giordano commented 3 years ago

I'm not opposed, I mean, I had opened a PR with exactly the same title some time ago :slightly_smiling_face: