featr: add vector type support (#330)

Add vector type support (ex: '100E') and fix bit handling * In the FITS spec, columns can be of vector type, where each row has multiple of the same data type. Bitmasks (`32X`) and strings (`20A`) are the most common examples, but others exist in the wild. My example is from [Planck Legacy Archives](https://pla.esac.esa.int/pla/index.html#home) → Maps → CMB Maps → Uncheck "Only legacy products" → SEVEM → Click to download the single row (full mission); this FITS file contains one row, which is `TFORM1` of `201326592E`, which happens to be a HEALPix map with `NSIDE` of 4096. * In more fully supporting repeats for vectors, revamped the bit handling. * Filled out the data type support and added round-trip read-write tests for all vector types. Weirdly, I couldn't get the u32 case to work and suspect a CFITSIO bug. I left the u32 test commented out for now with a more detailed explanation. Would love someone to reproduce and investigate. Then again, all unsigned integral types beyond u8 are unofficial CFITSIO extensions beyond the FITS spec, so maybe it's OK to leave out. * Removed conditionals for 32-bit and 64-bit `reads_col_impl` and `writes_col_impl`; `int` is 32 bits and `long long` is consistently 64 bits on all machines and platforms since ~2000. Only `long` is weird and special. Contribution checklist: * `cargo test` passes on my M1 Mac and on an x86_64 Linux machine using the provided Docker environment. I'm afraid I don't have any 32-bit or Windows environments available for testing. * `rustfmt` changes nothing. * Added to changelog. * I have not updated `full_example.rs`, as `test_vector_datatypes.rs` shows how to use vector types, but I am happy to do so upon request. --------- Co-authored-by: Simon Walker <[email protected]>
simonrw · Aug 6, 2024 · c9bece4 · c9bece4
1 parent b9a187e
commit c9bece4
Show file tree

Hide file tree

Showing 8 changed files with 217 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
 
 ## [Unreleased]
 ### Added
+* Support for vector data-types in reading and writing tables.
 ### Changed
 ### Removed
 

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -26,6 +26,7 @@ Before submitting a completed PR, make sure the following items have been addres
 * **format the code** - make sure the code has been formatted by `rustfmt` before submitting. I have a [git `pre-push` hook](https://gist.github.com/zofrex/4a5084c49e4aadd0a3fa0edda14b1fa8) which handles this for me.
 * **update the features tracking issue** - if relevant, update the [features tracking issue][features-tracking-issue]
 * **update the full example** - if new features have been added, or changes made, update the `full_example.rs` example
+* **satisfy clippy** - our Github CI will apply `cargo clippy` with warnings treated as errors.
 
 [features-tracking-issue]: https://github.com/simonrw/rust-fitsio/issues/15
 

diff --git a/fitsio/src/fitsfile.rs b/fitsio/src/fitsfile.rs
@@ -368,27 +368,29 @@ impl FitsFile {
                 for i in 0..num_cols {
                     let mut name_buffer: Vec<libc::c_char> = vec![0; 71];
                     let mut type_buffer: Vec<libc::c_char> = vec![0; 71];
+                    let mut repeats: libc::c_long = 0;
                     unsafe {
                         fits_get_bcolparms(
                             self.fptr.as_mut() as *mut _,
                             i + 1,
                             name_buffer.as_mut_ptr(),
                             ptr::null_mut(),
                             type_buffer.as_mut_ptr(),
-                            ptr::null_mut(),
+                            &mut repeats as *mut libc::c_long,
                             ptr::null_mut(),
                             ptr::null_mut(),
                             ptr::null_mut(),
                             ptr::null_mut(),
                             &mut status,
                         );
                     }
-
-                    column_descriptions.push(ConcreteColumnDescription {
+                    let mut col = ConcreteColumnDescription {
                         name: stringutils::buf_to_string(&name_buffer)?,
                         data_type: stringutils::buf_to_string(&type_buffer)?
                             .parse::<ColumnDataDescription>()?,
-                    });
+                    };
+                    col.data_type.repeat = repeats as usize;
+                    column_descriptions.push(col);
                 }
 
                 HduInfo::TableInfo {

diff --git a/fitsio/src/longnam.rs b/fitsio/src/longnam.rs
@@ -5,15 +5,16 @@
 #![allow(unused_imports, dead_code)]
 
 pub(crate) use crate::sys::{
-    ffclos, ffcopy, ffcrim, ffcrtb, ffdcol, ffdhdu, ffflmd, ffgbcl, ffgcdw, ffgcno, ffgcvd, ffgcve,
-    ffgcvi, ffgcvj, ffgcvjj, ffgcvk, ffgcvl, ffgcvs, ffgcvui, ffgcvuj, ffgcvujj, ffgcvuk, ffghdn,
-    ffghdt, ffgidm, ffgiet, ffgisz, ffgkyd, ffgkye, ffgkyj, ffgkyjj, ffgkyl, ffgkys, ffgncl,
-    ffgnrw, ffgpv, ffgsv, fficol, ffinit, ffmahd, ffmnhd, ffopen, ffpcl, ffpcls, ffphps, ffpky,
-    ffpkyd, ffpkye, ffpkys, ffppr, ffpss, ffrsim, ffthdu, fitsfile, LONGLONG,
+    ffclos, ffcopy, ffcrim, ffcrtb, ffdcol, ffdhdu, ffflmd, ffgbcl, ffgcdw, ffgcno, ffgcvb, ffgcvd,
+    ffgcve, ffgcvi, ffgcvj, ffgcvjj, ffgcvk, ffgcvl, ffgcvs, ffgcvsb, ffgcvui, ffgcvuj, ffgcvujj,
+    ffgcvuk, ffgcx, ffghdn, ffghdt, ffgidm, ffgiet, ffgisz, ffgkyd, ffgkye, ffgkyj, ffgkyjj,
+    ffgkyl, ffgkys, ffgncl, ffgnrw, ffgpv, ffgsv, fficol, ffinit, ffmahd, ffmnhd, ffopen, ffpcl,
+    ffpcls, ffpclx, ffphps, ffpky, ffpkyd, ffpkye, ffpkys, ffppr, ffpss, ffrsim, ffthdu, fitsfile,
+    LONGLONG,
 };
 pub use libc::{
-    c_char, c_double, c_float, c_int, c_long, c_short, c_uint, c_ulong, c_ulonglong, c_ushort,
-    c_void,
+    c_char, c_double, c_float, c_int, c_long, c_schar, c_short, c_uchar, c_uint, c_ulong,
+    c_ulonglong, c_ushort, c_void,
 };
 
 pub(crate) unsafe fn fits_close_file(fptr: *mut fitsfile, status: *mut libc::c_int) -> c_int {
@@ -148,6 +149,38 @@ pub(crate) unsafe fn fits_read_col_log(
     )
 }
 
+pub(crate) unsafe fn fits_read_col_byt(
+    fptr: *mut fitsfile,
+    colnum: c_int,
+    firstrow: LONGLONG,
+    firstelem: LONGLONG,
+    nelem: LONGLONG,
+    nulval: c_uchar,
+    array: *mut c_uchar,
+    anynul: *mut c_int,
+    status: *mut c_int,
+) -> c_int {
+    ffgcvb(
+        fptr, colnum, firstrow, firstelem, nelem, nulval, array, anynul, status,
+    )
+}
+
+pub(crate) unsafe fn fits_read_col_sbyt(
+    fptr: *mut fitsfile,
+    colnum: c_int,
+    firstrow: LONGLONG,
+    firstelem: LONGLONG,
+    nelem: LONGLONG,
+    nulval: c_schar,
+    array: *mut c_schar,
+    anynul: *mut c_int,
+    status: *mut c_int,
+) -> c_int {
+    ffgcvsb(
+        fptr, colnum, firstrow, firstelem, nelem, nulval, array, anynul, status,
+    )
+}
+
 pub(crate) unsafe fn fits_read_col_sht(
     fptr: *mut fitsfile,
     colnum: c_int,

diff --git a/fitsio/src/tables.rs b/fitsio/src/tables.rs
@@ -6,6 +6,7 @@ use crate::longnam::*;
 use crate::stringutils::status_to_string;
 use crate::types::DataType;
 use std::ffi;
+use std::mem::size_of;
 use std::ops::Range;
 use std::ptr;
 use std::str::FromStr;
@@ -59,7 +60,6 @@ macro_rules! reads_col_impl {
                         ..
                     }) => {
                         let num_output_rows = range.end - range.start;
-                        let mut out = vec![$nullval; num_output_rows];
                         let test_name = name.into();
                         let column_number = column_descriptions
                             .iter()
@@ -68,14 +68,24 @@ macro_rules! reads_col_impl {
                                 "Cannot find column {:?}",
                                 test_name
                             )))?;
+                        let col_desc = &column_descriptions[column_number];
+                        #[allow(clippy::manual_bits)]
+                        let repeat = if col_desc.data_type.typ == ColumnDataType::Bit {
+                            // take the maximum of hte value with 1 for data types smaller than 8
+                            // bites, e.g. u32
+                            (col_desc.data_type.repeat / (size_of::<$t>() * 8)).max(1)
+                        } else {
+                            col_desc.data_type.repeat
+                        };
+                        let mut out = vec![$nullval; num_output_rows * repeat];
                         let mut status = 0;
                         unsafe {
                             $func(
                                 fits_file.fptr.as_mut() as *mut _,
                                 (column_number + 1) as i32,
                                 (range.start + 1) as i64,
                                 1,
-                                num_output_rows as _,
+                                (num_output_rows * repeat) as _,
                                 $nullval,
                                 out.as_mut_ptr(),
                                 ptr::null_mut(),
@@ -121,6 +131,12 @@ macro_rules! reads_col_impl {
                                 "Cannot find column {:?}",
                                 test_name
                             )))?;
+                        let repeat = column_descriptions[column_number].data_type.repeat;
+                        if repeat > 1 {
+                            unimplemented!(
+                                "reading a single cell of a vector value (e.g., TFORM1 = 100E) is unimplemented. Call read_col() or read_col_range()."
+                            )
+                        }
                         let mut status = 0;
 
                         unsafe {
@@ -248,19 +264,15 @@ impl ReadsCol for bool {
     }
 }
 
+reads_col_impl!(u8, fits_read_col_byt, 0);
+reads_col_impl!(i8, fits_read_col_sbyt, 0);
 reads_col_impl!(i16, fits_read_col_sht, 0);
 reads_col_impl!(u16, fits_read_col_usht, 0);
 reads_col_impl!(i32, fits_read_col_int, 0);
 reads_col_impl!(u32, fits_read_col_uint, 0);
 reads_col_impl!(f32, fits_read_col_flt, 0.0);
 reads_col_impl!(f64, fits_read_col_dbl, 0.0);
-#[cfg(all(target_pointer_width = "64", not(target_os = "windows")))]
-reads_col_impl!(i64, fits_read_col_lng, 0);
-#[cfg(any(target_pointer_width = "32", target_os = "windows"))]
 reads_col_impl!(i64, fits_read_col_lnglng, 0);
-#[cfg(all(target_pointer_width = "64", not(target_os = "windows")))]
-reads_col_impl!(u64, fits_read_col_ulng, 0);
-#[cfg(any(target_pointer_width = "32", target_os = "windows"))]
 reads_col_impl!(u64, fits_read_col_ulnglng, 0);
 
 impl ReadsCol for String {
@@ -387,7 +399,7 @@ macro_rules! writes_col_impl {
                         let colno = hdu.get_column_no(fits_file, col_name.into())?;
                         // TODO: check that the column exists in the file
                         let mut status = 0;
-                        let n_elements = rows.end - rows.start;
+                        let n_elements = (rows.end - rows.start);
                         unsafe {
                             fits_write_col(
                                 fits_file.fptr.as_mut() as *mut _,
@@ -415,15 +427,13 @@ macro_rules! writes_col_impl {
     };
 }
 
+writes_col_impl!(u8, DataType::TBYTE);
+writes_col_impl!(i8, DataType::TSBYTE);
+writes_col_impl!(u16, DataType::TUSHORT);
+writes_col_impl!(i16, DataType::TSHORT);
 writes_col_impl!(u32, DataType::TUINT);
-#[cfg(all(target_pointer_width = "64", not(target_os = "windows")))]
-writes_col_impl!(u64, DataType::TULONG);
-#[cfg(any(target_pointer_width = "32", target_os = "windows"))]
-writes_col_impl!(u64, DataType::TLONGLONG);
 writes_col_impl!(i32, DataType::TINT);
-#[cfg(all(target_pointer_width = "64", not(target_os = "windows")))]
-writes_col_impl!(i64, DataType::TLONG);
-#[cfg(any(target_pointer_width = "32", target_os = "windows"))]
+writes_col_impl!(u64, DataType::TULONGLONG);
 writes_col_impl!(i64, DataType::TLONGLONG);
 writes_col_impl!(f32, DataType::TFLOAT);
 writes_col_impl!(f64, DataType::TDOUBLE);
@@ -639,14 +649,20 @@ impl From<ColumnDataDescription> for String {
 #[allow(missing_docs)]
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum ColumnDataType {
+    Logical,
     Bit,
-    Bool,
+    Byte,
+    SignedByte,
+    Short,
+    UnsignedShort,
     Int,
+    Long,
+    UnsignedLong,
     Float,
     Text,
     Double,
-    Short,
-    Long,
+    LongLong,
+    UnsignedLongLong,
     String,
 }
 
@@ -655,14 +671,19 @@ impl From<ColumnDataType> for String {
         use self::ColumnDataType::*;
 
         match orig {
+            Logical => "L",
             Bit => "X",
-            Bool => "B",
-            Int => "J",
+            Byte => "B",
+            SignedByte => "S",
+            Short => "I",
+            UnsignedShort => "U",
+            Int | Long => "J",
+            UnsignedLong => "V",
             Float => "E",
             Text | String => "A",
             Double => "D",
-            Short => "I",
-            Long => "K",
+            LongLong => "K",
+            UnsignedLongLong => "W",
         }
         .to_string()
     }
@@ -712,15 +733,19 @@ impl FromStr for ColumnDataDescription {
         };
 
         let data_type = match data_type_char {
+            'L' => ColumnDataType::Logical,
             'X' => ColumnDataType::Bit,
-            'B' => ColumnDataType::Bool,
+            'B' => ColumnDataType::Byte,
+            'S' => ColumnDataType::SignedByte,
             'E' => ColumnDataType::Float,
             'J' => ColumnDataType::Int,
             'D' => ColumnDataType::Double,
             'I' => ColumnDataType::Short,
-            'K' => ColumnDataType::Long,
+            'K' => ColumnDataType::LongLong,
             'A' => ColumnDataType::String,
-            'L' => ColumnDataType::Bool,
+            'U' => ColumnDataType::UnsignedShort,
+            'V' => ColumnDataType::UnsignedLong,
+            'W' => ColumnDataType::UnsignedLongLong,
             _ => panic!(
                 "Have not implemented str -> ColumnDataType for {}",
                 data_type_char
@@ -772,8 +797,9 @@ macro_rules! datatype_into_impl {
                     DataType::TINT => 31,
                     DataType::TULONG => 40,
                     DataType::TLONG => 41,
-                    DataType::TLONGLONG => 81,
                     DataType::TFLOAT => 42,
+                    DataType::TULONGLONG => 80,
+                    DataType::TLONGLONG => 81,
                     DataType::TDOUBLE => 82,
                     DataType::TCOMPLEX => 83,
                     DataType::TDBLCOMPLEX => 163,

diff --git a/fitsio/src/types.rs b/fitsio/src/types.rs
@@ -16,8 +16,9 @@ pub enum DataType {
     TINT,
     TULONG,
     TLONG,
-    TLONGLONG,
     TFLOAT,
+    TULONGLONG,
+    TLONGLONG,
     TDOUBLE,
     TCOMPLEX,
     TDBLCOMPLEX,

diff --git a/fitsio/tests/test_bit_datatype.rs b/fitsio/tests/test_bit_datatype.rs
@@ -19,10 +19,10 @@ fn test_writing_bit_data_type() {
     let tmp_dir = Builder::new().prefix("fitsio-").tempdir().unwrap();
     let file_path = tmp_dir.path().join("example.fits");
 
+    let data: Vec<u32> = (0..64).collect();
     {
         let mut fitsfile = FitsFile::create(&file_path).open().unwrap();
 
-        let data: Vec<u32> = (0..64).collect();
         let col = ColumnDescription::new("BITMASK")
             .with_type(ColumnDataType::Bit)
             .create()
@@ -39,4 +39,5 @@ fn test_writing_bit_data_type() {
     let table_hdu = f.hdu("DATA").unwrap();
     let flags: Vec<u32> = dbg!(table_hdu.read_col(&mut f, "BITMASK").unwrap());
     assert_eq!(flags.len(), 64);
+    assert!(data.iter().zip(&flags).all(|(a, b)| a == b));
 }