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 Table HDUs a Tables.jl source #151

Closed ajwheeler closed 3 years ago

ajwheeler commented 3 years ago

This moves the functionality of FITSTables.jl into FITSIO.jl, with a some improvements. It provides methods for the Tables "column table" functions, which makes it easy to, e.g. load a table HDU into a DataFrame (without introducing a DataFrames dep to this package).

A couple details:

ajwheeler commented 3 years ago

In this, I define const EitherTableHDU = Union{TableHDU, ASCIITableHDU}. I'm happy to either remove it / rename it / define it at the top of the file and use it throughout.

ajwheeler commented 3 years ago

This is an excellent point. Each array is indeed transposed.

In [19]: t = Table([[1, 2, 3], [2, 3, 1], np.array(range(12)).reshape(3, 2, 2)], names=("a", "b", "c"))

In [20]: t
Out[20]: 
<Table length=3>
  a     b   c [2,2]
int64 int64  int64 
----- ----- -------
    1     2  0 .. 3
    2     3  4 .. 7
    3     1 8 .. 11

In [21]: t["c"][0]
Out[21]: 
array([[0, 1],
       [2, 3]])

In [22]: t.write("test2.fits", format="fits")
julia> FITS(DataFrame ∘ last, "test2.fits")
3×3 DataFrame
│ Row │ a     │ b     │ c            │
│     │ Int64 │ Int64 │ Array…       │
├─────┼───────┼───────┼──────────────┤
│ 1   │ 1     │ 2     │ [0 2; 1 3]   │
│ 2   │ 2     │ 3     │ [4 6; 5 7]   │
│ 3   │ 3     │ 1     │ [8 10; 9 11] │
ajwheeler commented 3 years ago

This potential point of confusion is not specific to the Tables.jl methods. Maybe it should be more prominent in the docs?

julia> FITS("test2.fits") do f
           read(f[2], "c")end
2×2×3 Array{Int64,3}:
[:, :, 1] =
 0  2
 1  3

[:, :, 2] =
 4  6
 5  7

[:, :, 3] =
 8  10
 9  11
mileslucas commented 3 years ago

Yeah this is the really unfortunate effect of using CFITSIO- the arrays are loaded into memory in row-first and transfer the pointers to Julia which assumes column first. It's the exact same problem we have with reading any FITS data and its in all of the FITS tables; this isn't specific to the Tables.jl interface. I wanted to ensure this was consistent, and we do need more explicit documentation about this issue in general.

Can you add a note about this in the docs, probably before or after the ragged array comment. Something like "any arrays within the table will appear transposed/permuted from what is expected due to the memory interop of CFITSIO and Julia".

codecov[bot] commented 3 years ago

Codecov Report

Merging #151 (bf8c39c) into master (51e4c6a) will increase coverage by 0.74%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   88.07%   88.81%   +0.74%     
==========================================
  Files           5        5              
  Lines         612      626      +14     
==========================================
+ Hits          539      556      +17     
+ Misses         73       70       -3     
Impacted Files Coverage Δ
src/FITSIO.jl 100.00% <ø> (ø)
src/table.jl 90.41% <100.00%> (+2.11%) :arrow_up:

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 51e4c6a...bf8c39c. Read the comment docs.