Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 109 additions & 2 deletions fitsio/src/hdu.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
//! Fits HDU related code

use crate::errors::{check_status, Result};
use fitsio_sys::{ffghsp, ffgkyn};

use crate::errors::{check_status, Error, Result};
use crate::fitsfile::CaseSensitivity;
use crate::fitsfile::FitsFile;
use crate::headers::card::Card;
use crate::headers::{ReadsKey, WritesKey};
use crate::images::{ImageType, ReadImage, WriteImage};
use crate::longnam::*;
use crate::tables::{
ColumnIterator, ConcreteColumnDescription, DescribesColumnLocation, FitsRow, ReadsCol,
WritesCol,
};
use std::ffi;
use std::{ffi, ptr};
use std::ops::Range;

/// Struct representing a FITS HDU
Expand Down Expand Up @@ -66,6 +69,11 @@ impl FitsHdu {
T::read_key(fits_file, name)
}

/// Return cards available in the HDU header.
pub fn cards(&self, fits_file: &mut FitsFile) -> Result<CardIter> {
CardIter::new(fits_file, self)
}

/**
Write a fits key to the current header

Expand Down Expand Up @@ -1062,11 +1070,99 @@ hduinfo_into_impl!(i8);
hduinfo_into_impl!(i32);
hduinfo_into_impl!(i64);

/// Iterator for the keys of an HDU header.
pub struct CardIter {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to update read_key to support either a name or index, and support returning the Card type so we can return the key as well. Then the iterator would boil down to something like

// TODO: proper types
struct CardIter {
    idx: usize,
    num_cards: usize,
    fits_file: FitsFile,
    hdu: FitsHdu,
}

impl Iterator for CardIter {
    type Item = Card;

    fn next(&mut self) -> Option<Self::Item> {
        if self.idx >= self.num_cards {
            return None;
        }

        // Generic return type means we can implement `ReadsKey` for `Card` which returns the name, value and (optional) comment
        let card: Card = self.hdu.read_key(&mut self.fits_file, self.idx).unwrap();  // TODO: error handling
        Some(card)
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds right, I am new to the both FITS and the fitsio API and missed that this was a possibility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read the section 4 in the FITS standard related to headers and realised that long strings are not supported neither by fits_read_keyn nor fits_read_record routine, but I am guessing that the latter would return CONTINUE records that allows the piecing together of long strings.

If we want support for long strings from the beginning we must use either ffgrec or read all headers using fits_hdr2str as mentioned in your last comment in #374.

fptr: *mut fitsfile,
total: c_int,
current: c_int,
// name: [i8;FLEN_KEYWORD as usize],
// value: [i8;FLEN_VALUE as usize],
// comment: [i8;FLEN_COMMENT as usize],
status: c_int,
}

impl CardIter {
fn new(file: &mut FitsFile, hdu: &FitsHdu) -> Result<CardIter> {
file.make_current(hdu)?;

let mut iter = CardIter {
fptr: unsafe { file.as_raw() },
total: 0,
current: 1,
// name: [0i8;FLEN_KEYWORD as usize],
// value: [0i8;FLEN_VALUE as usize],
// comment: [0i8;FLEN_COMMENT as usize],
status: 0,
};

let mut nmore: c_int = 0;
unsafe {
ffghsp(
iter.fptr,
ptr::addr_of_mut!(iter.total),
ptr::addr_of_mut!(nmore),
ptr::addr_of_mut!(iter.status)
)
};
if iter.total == 0 {
Err(Error::Message("no headers available".to_owned()))
} else {
iter.next_header()?; // reset the header count by using index 0
Ok(iter) // iterator now points at first header card
}
}
fn next_header(&mut self) -> Result<Card> {
self.status = 0; // reset the status before calling
let mut card = Card::default();
unsafe {
ffgkyn(
self.fptr,
self.current,
// self.name.as_mut_ptr(),
// self.value.as_mut_ptr(),
// self.comment.as_mut_ptr(),
card.name.as_mut_ptr() as *mut c_char,
card.value.as_mut_ptr() as *mut c_char,
card.comment.as_mut_ptr() as *mut c_char,
ptr::addr_of_mut!(self.status)
)
};
self.current += 1;
check_status(self.status)?;
Ok(card)
}
}

impl Iterator for CardIter {
type Item = Card;

fn next(&mut self) -> Option<Self::Item> {
if self.current > self.total {
return None
}
match self.next_header() {
Ok(card) => Some(card),
Err(e) => {
let mut card = Card::default();
card.set_comment(format!("{e}"));
Some(card)
},
}

// if let Err(e) = self.next_header() {
// Some(format!("{e}")) // return the error as the keyword
// } else {
// buf_to_string(self.name.as_slice()).ok()
// }
}
}

#[cfg(test)]
mod tests {
use super::FitsFile;
use crate::hdu::{FitsHdu, HduInfo};
use crate::testhelpers::duplicate_test_file;
use crate::errors::Result;

#[test]
fn test_manually_creating_a_fits_hdu() {
Expand Down Expand Up @@ -1098,6 +1194,16 @@ mod tests {
assert_eq!(intcol_data[49], 12);
}

#[test]
fn test_cards() -> Result<()> {
let mut f = FitsFile::open("../testdata/full_example.fits").unwrap();
let primary_hdu = f.hdu(0).unwrap();
for card in primary_hdu.cards(&mut f).unwrap() {
println!("{:8} = {:10} - {}", card.name()?, card.str_value()?, card.comment()?);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course this test doesn't actually test anything, let's revisit when we have agreement on the overall design.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, indeed. Just wanted proof that the code could run without hitting a lifetime snag or similar.

}
Ok(())
}

#[test]
fn test_fetch_hdu_name() {
duplicate_test_file(|filename| {
Expand All @@ -1106,6 +1212,7 @@ mod tests {
assert_eq!(hdu.name(&mut f).unwrap(), "TESTEXT".to_string());
});
}

#[test]
fn test_delete_hdu() {
duplicate_test_file(|filename| {
Expand Down
55 changes: 55 additions & 0 deletions fitsio/src/headers/card.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//! Fits HDU related code

use fitsio_sys::{FLEN_COMMENT, FLEN_KEYWORD, FLEN_VALUE};

use crate::errors::Result;
use std::ffi::{c_char, CStr};

/// Wraps a single header card
#[derive(Debug)]
pub struct Card {
pub(crate) name: [i8;FLEN_KEYWORD as usize],
pub(crate) value: [i8;FLEN_VALUE as usize],
pub(crate) comment: [i8;FLEN_COMMENT as usize],
Comment on lines +11 to +13
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see why these methods are here in this implementation (lazily converting the types if required, rather than eagerly converting the types at Card creation) however I prefer doing the work up front:

  • the Card type is really a data object, with no inherent behaviour. As such, it really should just be a wrapper around the state
  • if the user wants the Card then they likely want at least two of the fields (name and value) so most of the conversion work is not wasted
  • the conversion is not exactly intensive so it probably not a performance issue right now.

Copy link
Copy Markdown
Contributor Author

@chrsoo chrsoo Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name and comment will always be strings, but given that we don't know what type the value is we cannot do the conversion upfront and the data structure must support all possible value types. This leaves us with two options:

  • Using a the i8 array as-is; or
  • Introducing a enum with variants of each type carrying a value.

I am still quite unfamiliar with the details of the fitsio API but I don't think we have that enum and the generic return types of read_key points to the first option with the buffer.

The struct would thus change to

pub(crate) name:       String,
pub(crate) comment:    String,
pub(crate) value:      [i8;FLEN_VALUE as usize],

Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better yet:

pub(crate) name:       String,
pub(crate) comment:    Option<String>,
pub(crate) value:      [i8;FLEN_VALUE as usize],

... given that comments are optional.

Copy link
Copy Markdown
Contributor Author

@chrsoo chrsoo Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • the Card type is really a data object, with no inherent behaviour. As such, it really should just be a wrapper around the state

The raw ì8 buffers are still state, just not parsed state... ;-)

Regarding doing conversion work up front, using an enum for values is only an option if we accept that we cannot distinguish between integers and float. Which one to go for, must be a choice of the code consuming the value. Presumably based on prior knowledge of the semantics of the header.

What we can do though, is to parse the raw data structures up front into safe code. Given that the iterator can only use one (concrete) type parameter we could have the following definition:

pub struct Card<T> {
    pub(crate) name:       String,
    pub(crate) comment:    Option<String>,
    pub(crate) value:      T,
}

... and use a generic CardIter<String> for HDU.cards().

I think this should work fine as, iterating over the cards is something I would expect we do only when we do not need a specific keyword. Any type conversions can be done in a next step.

If we do need a specific keyword, we should know the type and can retrieve the converted keyword by name .

@simonrw unless you see any issues with this approach, this is what I will go for.

If we want to be a bit fancy, the HDU.cards() signature could take a generic type and we would gracefully handle all failed conversions. This would effectively be filtering by keyword type.

}

impl Card {
/// Header keyword.
pub fn name(&self) -> Result<&str> {
Ok(unsafe { CStr::from_ptr(self.name.as_ptr() as *mut c_char) }.to_str()?)
}

/// Header comment.
pub fn comment(&self) -> Result<&str> {
Ok(unsafe { CStr::from_ptr(self.comment.as_ptr() as *mut c_char) }.to_str()?)
}

/// Header value as a &str without enclosing quotes.
pub fn str_value(&self) -> Result<&str> {
let cstr = unsafe { CStr::from_ptr(self.value.as_ptr() as *mut c_char) };
let str = cstr.to_str()?.trim_matches('\'');
Ok(str)
}

pub(crate) fn set_comment(&mut self, comment: String) {
self.comment.fill(0); // clear the buffer before using it, ensure null termination
let mut i = 0;
for b in comment.into_bytes() {
self.comment[i] = b as i8;
i += 1;
if i >= self.comment.len() - 1 { // C string must be null terminated
break
}
}
}
}

impl Default for Card {
fn default() -> Self {
Card {
name: [0i8;FLEN_KEYWORD as usize],
value: [0i8;FLEN_VALUE as usize],
comment: [0i8;FLEN_COMMENT as usize],
}
}
}
1 change: 1 addition & 0 deletions fitsio/src/headers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::types::DataType;
use std::ffi;
use std::ptr;

pub mod card;
mod constants;
mod header_value;

Expand Down