travis clone depth

No fallible casts

.travis.yml

@@ -0,0 +1,9 @@
+language: rust
+rust:
+  - stable
+  - beta
+  - nightly
+git:
+  depth: 50
+after_success:
+  ./.travis_bench.sh

.travis_bench.sh

@@ -0,0 +1,21 @@
+#!/usr/bin/env bash
+
+# Clone the repository
+REMOTE_URL="$(git config --get remote.origin.url)";
+cd ${TRAVIS_BUILD_DIR}/.. && \
+git clone ${REMOTE_URL} "${TRAVIS_REPO_SLUG}-bench" && \
+cd  "${TRAVIS_REPO_SLUG}-bench" && \
+
+# Bench master
+git checkout master && \
+cargo bench -- --noplot --save-baseline before && \
+
+# Bench current branch
+git checkout ${TRAVIS_COMMIT} && \
+cargo bench -- --noplot --save-baseline after && \
+
+# Install https://github.com/BurntSushi/critcmp
+cargo install critcmp --force && \
+
+# Compare the two generated benches
+critcmp before after;

Cargo.toml

@@ -1,10 +1,14 @@
 [package]
 name = "xdrfile"
-version = "0.2.0"
+version = "0.3.0"
 authors = ["Daniel Bauer <bauer@cbs.tu-darmstadt.de>"]
 license = "LGPL-3.0-only"
 edition = "2018"
+repository = "https://github.com/danijoo/xdrfile"
 description = "Wrapper around the gromacs libxdrfile library. Can be used to read and write gromacs trajectories in xtc and trr format."
+readme = "README.md"
+categories = ["external-ffi-bindings", "encoding", "science"]
+keywords = ["bioinformatics", "molecular-dynamics", "GROMACS", "trajectory"]
 build = "build.rs"
 
 [dependencies]
@@ -13,6 +17,14 @@ lazy-init = "0.3"
 [dev-dependencies]
 tempfile = "3.1.0"
 assert_approx_eq = "1.1.0"
+criterion = "0.3"
 
 [build-dependencies]
 cc = { version = "1.0", features = ["parallel" ]}
+
+[lib]
+bench = false
+
+[[bench]]
+name = "benchmarks"
+harness = false

README.md

@@ -1,4 +1,4 @@
-[![crates](https://img.shields.io/badge/crates.io-v0.1.0-orange.svg?longCache=true)](https://crates.io/crates/xdrfile)
+[![Build Status](https://travis-ci.org/danijoo/xdrfile.svg?branch=master)](https://travis-ci.org/danijoo/xdrfile) [![crates.io](https://img.shields.io/badge/crates.io-orange.svg?longCache=true)](https://www.crates.io/crates/xdrfile) [![Latest Documentation](https://docs.rs/xdrfile/badge.svg)](https://docs.rs/xdrfile)
 
 # xdrfile
 Read and write xdr trajectory files in .xtc and .trr file format
@@ -11,32 +11,28 @@ files with a safe api.
 ### Basic usage
 ```rust
 use xdrfile::*;
-use std::path::Path; 
 
-let mut path = Path::new("tests/1l2y.xtc");
+fn main() -> Result<()> {
-// get a handle to the file
+    // get a handle to the file
-let mut trj = XTCTrajectory::open(path, FileMode::Read).unwrap();
+    let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
 
-// find number of atoms in the file
+    // find number of atoms in the file
-let num_atoms = trj.get_num_atoms().unwrap();
+    let num_atoms = trj.get_num_atoms()?;
 
-// a frame object is used to get to read or write from a trajectory
+    // a frame object is used to get to read or write from a trajectory
-// without instantiating data arrays for every step
+    // without instantiating data arrays for every step
-let mut frame = Frame::with_capacity(num_atoms);
+    let mut frame = Frame::with_len(num_atoms);
 
-// read the first frame of the trajectory
+    // read the first frame of the trajectory
-let result = trj.read(&mut frame);
+    trj.read(&mut frame)?;
-match result {
-   Ok(_) => {
-       assert_eq!(frame.step, 1);
-       assert_eq!(frame.num_atoms, num_atoms);
 
-       let first_atom_coords = frame.coords[0];
+    assert_eq!(frame.step, 1);
-       assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]);
+    assert_eq!(frame.len(), num_atoms);
-   }
+
-   Err(msg) => {
+    let first_atom_coords = frame[0];  // shorthand for frame.coords[0]
-       panic!("Something went wrong: {}", msg);    
+    assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]);
-   }
+
+    Ok(())
 }
 ```
 
@@ -48,16 +44,18 @@ Rc is required)
 
 ```rust
 use xdrfile::*;
-use std::path::Path; 
 
-let mut path = Path::new("tests/1l2y.xtc");
+fn main() -> Result<()> {
-// get a handle to the file
+    // get a handle to the file
-let trj = XTCTrajectory::open(path, FileMode::Read).unwrap();
+    let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
 
-// iterate over all frames
+    // iterate over all frames
-for (idx, frame) in trj.into_iter().filter_map(Result::ok).enumerate() {
+    for (idx, result) in trj.into_iter().enumerate() {
-    println!("{}", frame.time);
+        let frame = result?;
-    assert_eq!(idx+1, frame.step as usize);
+        println!("{}", frame.time);
+        assert_eq!(idx+1, frame.step);
+    }
+    Ok(())
 }
 ```
 

benches/benchmarks.rs

@@ -0,0 +1,75 @@
+use std::rc::Rc;
+use tempfile::NamedTempFile;
+use xdrfile::*;
+use criterion::{black_box, criterion_group, criterion_main, Criterion};
+use std::time::Duration;
+
+/// generate a temporary test trajectory of given length
+fn gen_test_traj(num_atoms: usize, num_frames: usize) -> Result<NamedTempFile> {
+    let tempfile = NamedTempFile::new().expect("Could not create temporary file");
+    let tmp_path = tempfile.path().to_path_buf();
+    let mut f = XTCTrajectory::open_write(&tmp_path)?;
+
+    let mut frame = Frame {
+        step: 1,
+        time: 1.0,
+        box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]],
+        coords: vec![[1.0, 1.1, 1.2]; num_atoms],
+    };
+
+    for _ in 0..num_frames {
+        for j in 0..num_atoms {
+            frame.coords[j][0] += 1.0;
+            frame.coords[j][1] += 1.0;
+            frame.coords[j][2] += 1.0;
+            frame.time += 1.0;
+            frame.step += 1;
+        }
+        f.write(&frame)?;
+    }
+    f.flush()?;
+ 
+    Ok(tempfile)
+}
+
+// Iterate over a trajectory and do some stuff on it
+fn iterate_traj(file: &NamedTempFile, num_atoms: usize) -> Result<()> {
+    let path = file.path();
+    let traj = XTCTrajectory::open_read(path)?;
+    for frame in traj.into_iter() {
+        assert_eq!(frame?.len(), num_atoms);
+    }
+    Ok(())
+}
+
+// Iterate over a trajectory and keep_frames its frames
+// This benchmarks iteration speed when frames are not reused
+fn iterate_traj_keep_frames(file: &NamedTempFile, num_frames: usize) -> Result<()> {
+    let path = file.path();
+    let traj = XTCTrajectory::open_read(path)?;
+    let frames = traj.into_iter().filter_map(Result::ok).collect::<Vec<Rc<Frame>>>();
+    assert_eq!(num_frames, frames.len());
+    Ok(())
+}
+
+fn bench_iterate_traj(c: &mut Criterion) {
+    let num_atoms = 100;
+    let num_frames = 1000;
+    let tempfile = gen_test_traj(num_atoms, num_frames).unwrap();
+
+    let mut group = c.benchmark_group("iterate_traj");
+    group.significance_level(0.05)
+         .warm_up_time(Duration::from_secs(10))
+         .sample_size(2500)
+         .noise_threshold(0.05);  // high noise thresholds because of disk i/o
+    group.bench_function("iterate_traj", |b| b.iter(|| {
+        iterate_traj(black_box(&tempfile), black_box(num_atoms)).unwrap()
+    }));
+    group.bench_function("iterate_traj_keep_frames", |b| b.iter(|| {
+        iterate_traj_keep_frames(black_box(&tempfile), black_box(num_frames)).unwrap()
+    }));
+}
+
+criterion_group!(benches, bench_iterate_traj);
+criterion_main!(benches);
+

build.rs

@@ -1,17 +1,17 @@
 extern crate cc;
 
 use std::fs;
+use std::io::Result;
 
-fn main() {
+fn main() -> Result<()> {
     // This builds gromacs' xdrfile library
-    let source_files: Vec<_> = fs::read_dir("external/xdrfile/src")
+    let source_files = fs::read_dir("external/xdrfile/src")?
-        .unwrap()
+        .map(|r| r.map(|f| f.path()))
-        .map(|f| f.unwrap())
+        .collect::<Result<Vec<_>>>()?;
-        .map(|f| f.path())
-        .collect();
     cc::Build::new()
         .files(source_files)
         .include("external/xdrfile/include")
         .warnings(false)
-        .compile("libxdrfile.a")
+        .compile("libxdrfile.a");
+    Ok(())
 }

src/c_abi/mod.rs

@@ -1,3 +1,4 @@
+//! # Low level bindings to the c library from GROMACS
 #![allow(non_upper_case_globals, non_camel_case_types)]
 
 pub mod xdr_seek;

src/c_abi/xdr_seek.rs

@@ -111,8 +111,8 @@ mod tests {
     use std::ffi::CString;
 
     #[test]
-    fn test_xdr_tell() {
+    fn test_xdr_tell() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.xtc").unwrap();
+        let path = CString::new("tests/1l2y.xtc")?;
         let num_atoms = 304;
         let mut time: f32 = 2.0;
         let mut step: i32 = 5;
@@ -121,7 +121,7 @@ mod tests {
         let mut prec: f32 = 0.0;
 
         unsafe {
-            let mode = CString::new("r").unwrap();
+            let mode = CString::new("r")?;
             let xdr = xdrfile_open(path.as_ptr(), mode.as_ptr());
             assert!(!xdr.is_null());
 
@@ -140,15 +140,16 @@ mod tests {
 
             let tell = xdr_tell(xdr);
             assert!(tell > 0, "{}", tell);
-        }
+        };
+        Ok(())
     }
 
     #[test]
-    fn test_xdr_seek() {
+    fn test_xdr_seek() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.xtc").unwrap();
+        let path = CString::new("tests/1l2y.xtc")?;
 
         unsafe {
-            let mode = CString::new("r").unwrap();
+            let mode = CString::new("r")?;
             let xdr = xdrfile_open(path.as_ptr(), mode.as_ptr());
             assert!(!xdr.is_null());
 
@@ -160,5 +161,6 @@ mod tests {
             let tell = xdr_tell(xdr);
             assert!(tell == 500, "{}", tell);
         }
+        Ok(())
     }
 }

src/c_abi/xdrfile.rs

@@ -6,21 +6,20 @@ pub struct XDRFILE {
     _unused: [u8; 0],
 }
 
-pub type BindgenTy1 = u32;
+pub const exdrOK: i32 = 0;
-pub const exdrOK: BindgenTy1 = 0;
+pub const exdrHEADER: i32 = 1;
-pub const exdrHEADER: BindgenTy1 = 1;
+pub const exdrSTRING: i32 = 2;
-pub const exdrSTRING: BindgenTy1 = 2;
+pub const exdrDOUBLE: i32 = 3;
-pub const exdrDOUBLE: BindgenTy1 = 3;
+pub const exdrINT: i32 = 4;
-pub const exdrINT: BindgenTy1 = 4;
+pub const exdrFLOAT: i32 = 5;
-pub const exdrFLOAT: BindgenTy1 = 5;
+pub const exdrUINT: i32 = 6;
-pub const exdrUINT: BindgenTy1 = 6;
+pub const exdr3DX: i32 = 7;
-pub const exdr3DX: BindgenTy1 = 7;
+pub const exdrCLOSE: i32 = 8;
-pub const exdrCLOSE: BindgenTy1 = 8;
+pub const exdrMAGIC: i32 = 9;
-pub const exdrMAGIC: BindgenTy1 = 9;
+pub const exdrNOMEM: i32 = 10;
-pub const exdrNOMEM: BindgenTy1 = 10;
+pub const exdrENDOFFILE: i32 = 11;
-pub const exdrENDOFFILE: BindgenTy1 = 11;
+pub const exdrFILENOTFOUND: i32 = 12;
-pub const exdrFILENOTFOUND: BindgenTy1 = 12;
+pub const exdrNR: i32 = 13;
-pub const exdrNR: BindgenTy1 = 13;
 
 extern "C" {
     pub static mut exdr_message: [*mut ::std::os::raw::c_char; 13usize];

src/c_abi/xdrfile_trr.rs

@@ -47,32 +47,39 @@ mod tests {
     use tempfile::NamedTempFile;
 
     #[test]
-    fn test_read_trr_natoms() {
+    fn test_read_trr_natoms() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.trr").unwrap();
+        let path = CString::new("tests/1l2y.trr")?;
 
         let mut natoms = 0;
         unsafe {
-            read_trr_natoms(path.as_ptr() as *const i8, &mut natoms);
+            read_trr_natoms(path.as_ptr(), &mut natoms);
         }
         assert!(natoms == 304);
+        Ok(())
     }
 
     #[test]
-    fn test_read_trr_nframes() {
+    fn test_read_trr_nframes() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.trr").unwrap();
+        let path = CString::new("tests/1l2y.trr")?;
         let mut nframes: u64 = 0;
 
         unsafe {
-            let code = read_trr_nframes(path.as_ptr() as *const i8, &mut nframes);
+            let code = read_trr_nframes(path.as_ptr(), &mut nframes);
-            assert!(code as u32 == exdrOK);
+            assert!(code == exdrOK);
         }
         assert!(nframes == 38, "{:?}", nframes);
+        Ok(())
     }
 
     #[test]
-    fn test_read_write_trr() {
+    fn test_read_write_trr() -> Result<(), Box<dyn std::error::Error>> {
-        let tempfile = NamedTempFile::new().unwrap();
+        let tempfile = NamedTempFile::new()?;
-        let tmp_path = CString::new(tempfile.path().to_str().unwrap()).unwrap();
+        let tmp_path = CString::new(
+            tempfile
+                .path()
+                .to_str()
+                .expect("Could not convert path to str"),
+        )?;
 
         // write atoms to tempfile
         let natoms: i32 = 2;
@@ -86,7 +93,7 @@ mod tests {
         let f: Vec<Rvec> = vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]];
 
         unsafe {
-            let mode = CString::new("w").unwrap();
+            let mode = CString::new("w")?;
             let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr());
             let write_code = write_trr(
                 xdr,
@@ -99,7 +106,7 @@ mod tests {
                 v.as_ptr() as *mut Rvec,
                 f.as_ptr() as *mut Rvec,
             );
-            assert!(write_code as u32 == exdrOK);
+            assert!(write_code == exdrOK);
             xdrfile_close(xdr);
         }
 
@@ -114,7 +121,7 @@ mod tests {
         let f2: Vec<Rvec> = vec![[0.0, 0.0, 0.0]; 2];
 
         unsafe {
-            let mode = CString::new("r").unwrap();
+            let mode = CString::new("r")?;
             let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr());
             let read_code = read_trr(
                 xdr,
@@ -127,7 +134,7 @@ mod tests {
                 v2.as_ptr() as *mut Rvec,
                 f2.as_ptr() as *mut Rvec,
             );
-            assert!(read_code as u32 == exdrOK);
+            assert!(read_code == exdrOK);
             xdrfile_close(xdr);
         }
 
@@ -139,5 +146,6 @@ mod tests {
         assert!(x2 == x);
         assert!(v2 == v);
         assert!(f2 == f);
+        Ok(())
     }
 }

src/c_abi/xdrfile_xtc.rs

@@ -29,8 +29,8 @@ extern "C" {
         natoms: ::std::os::raw::c_int,
         step: ::std::os::raw::c_int,
         time: ::std::os::raw::c_float,
-        box_vec: *mut Matrix,
+        box_vec: *const Matrix,
-        x: *mut Rvec,
+        x: *const Rvec,
         prec: ::std::os::raw::c_float,
     ) -> ::std::os::raw::c_int;
 }
@@ -43,32 +43,39 @@ mod tests {
     use tempfile::NamedTempFile;
 
     #[test]
-    fn test_read_xtc_natoms() {
+    fn test_read_xtc_natoms() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.xtc").unwrap();
+        let path = CString::new("tests/1l2y.xtc")?;
 
         let mut natoms = 0;
         unsafe {
             read_xtc_natoms(path.as_ptr() as *mut i8, &mut natoms);
         }
         assert!(natoms == 304);
+        Ok(())
     }
 
     #[test]
-    fn test_read_xtc_nframes() {
+    fn test_read_xtc_nframes() -> Result<(), Box<dyn std::error::Error>> {
-        let path = CString::new("tests/1l2y.xtc").unwrap();
+        let path = CString::new("tests/1l2y.xtc")?;
         let mut nframes: u64 = 0;
 
         unsafe {
-            let code = read_xtc_nframes(path.as_ptr() as *const i8, &mut nframes);
+            let code = read_xtc_nframes(path.as_ptr(), &mut nframes);
-            assert!(code as u32 == exdrOK);
+            assert!(code == exdrOK);
         }
         assert!(nframes == 38, "{:?}", nframes);
+        Ok(())
     }
 
     #[test]
-    fn test_read_write_xtc() {
+    fn test_read_write_xtc() -> Result<(), Box<dyn std::error::Error>> {
-        let tempfile = NamedTempFile::new().unwrap();
+        let tempfile = NamedTempFile::new()?;
-        let tmp_path = CString::new(tempfile.path().to_str().unwrap()).unwrap();
+        let tmp_path = CString::new(
+            tempfile
+                .path()
+                .to_str()
+                .expect("Could not convert path to str"),
+        )?;
 
         // write atoms to tempfile
         let natoms: i32 = 2;
@@ -78,7 +85,7 @@ mod tests {
         let x: Vec<Rvec> = vec![[1.0, 1.0, 1.0], [1.0, 1.0, 1.0]];
 
         unsafe {
-            let mode = CString::new("w").unwrap();
+            let mode = CString::new("w")?;
             let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr());
             let write_code = write_xtc(
                 xdr,
@@ -89,7 +96,7 @@ mod tests {
                 x.as_ptr() as *mut Rvec,
                 1000.0,
             );
-            assert!(write_code as u32 == exdrOK);
+            assert!(write_code == exdrOK);
             xdrfile_close(xdr);
         }
 
@@ -101,7 +108,7 @@ mod tests {
         let mut prec: f32 = 0.0;
 
         unsafe {
-            let mode = CString::new("r").unwrap();
+            let mode = CString::new("r")?;
             let xdr = xdrfile_open(tmp_path.as_ptr(), mode.as_ptr());
             let read_code = read_xtc(
                 xdr,
@@ -112,7 +119,7 @@ mod tests {
                 x2.as_ptr() as *mut Rvec,
                 &mut prec,
             );
-            assert!(read_code as u32 == exdrOK);
+            assert!(read_code == exdrOK);
             xdrfile_close(xdr);
         }
 
@@ -121,5 +128,6 @@ mod tests {
         assert!(time2 == time);
         assert!(box_vec2 == box_vec);
         assert!(x2 == x);
+        Ok(())
     }
 }

src/errors.rs

@@ -0,0 +1,311 @@
+use crate::c_abi;
+use crate::FileMode;
+use crate::Frame;
+use std::error::Error as StdError;
+use std::path::{Path, PathBuf};
+
+/// Error type for the xdrfile library
+#[derive(Debug, Clone, PartialEq)]
+pub enum Error {
+    /// An error code from the C API
+    CApiError { code: ErrorCode, task: ErrorTask },
+    /// Passed in a frame of the wrong size
+    WrongSizeFrame { expected: usize, found: usize },
+    /// C API failed to open a file (No return code provided)
+    CouldNotOpen { path: PathBuf, mode: FileMode },
+    /// A path could not be converted to &OsStr
+    InvalidOsStr(Option<std::ffi::NulError>),
+    /// Checking the number of atoms failed while reading a frame
+    CouldNotCheckNAtoms(Box<Error>),
+    /// Error for an out-of-range numeric conversion
+    OutOfRange {
+        name: &'static str,
+        task: ErrorTask,
+        value: String,
+        target: &'static str,
+    },
+}
+
+impl Error {
+    /// Get the error code returned by the C API, if any
+    pub fn code(&self) -> Option<ErrorCode> {
+        if let Error::CApiError { code, .. } = self {
+            Some(*code)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::code)
+        } else {
+            None
+        }
+    }
+
+    /// Get the task being attempted when the C API returned an error, if any
+    pub fn task(&self) -> Option<ErrorTask> {
+        if let Error::CApiError { task, .. } = self {
+            Some(*task)
+        } else if let Some(e) = self.source() {
+            e.downcast_ref::<Self>().and_then(Self::task)
+        } else {
+            None
+        }
+    }
+
+    /// True if the error is an end of file error, false otherwise
+    pub fn is_eof(&self) -> bool {
+        self.code().map_or(false, |e| e.is_eof())
+    }
+}
+
+impl std::error::Error for Error {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match &self {
+            Error::InvalidOsStr(err) => {
+                if let Some(err) = err {
+                    Some(err)
+                } else {
+                    None
+                }
+            }
+            Error::CouldNotCheckNAtoms(err) => Some(err.as_ref()),
+            _ => None,
+        }
+    }
+}
+
+impl From<(ErrorCode, ErrorTask)> for Error {
+    fn from(value: (ErrorCode, ErrorTask)) -> Self {
+        let (code, task) = value;
+        Self::CApiError { code, task }
+    }
+}
+
+impl From<(&Path, FileMode)> for Error {
+    fn from(value: (&Path, FileMode)) -> Self {
+        let (path, mode) = value;
+        Error::CouldNotOpen {
+            path: path.to_owned(),
+            mode,
+        }
+    }
+}
+
+impl From<(&Frame, usize)> for Error {
+    fn from(value: (&Frame, usize)) -> Self {
+        let (frame, num_atoms) = value;
+        Error::WrongSizeFrame {
+            expected: num_atoms,
+            found: frame.coords.len(),
+        }
+    }
+}
+
+impl std::fmt::Display for Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Error::CApiError { code, task } => write!(
+                f,
+                "Error while {task}: C API returned error code {code}",
+                task = task,
+                code = code
+            ),
+            Error::WrongSizeFrame { expected, found } => write!(
+                f,
+                "Expected frame of size {:?}, found {:?}",
+                expected, found
+            ),
+            Error::CouldNotOpen { path, mode } => {
+                write!(f, "Could not open file at {:?} in mode {:?}", path, mode)
+            }
+            Error::InvalidOsStr(_) => write!(f, "Cannot convert path to CString."),
+            Error::CouldNotCheckNAtoms(_) => {
+                write!(f, "Failed to read number of atoms in trajectory file")
+            }
+            Error::OutOfRange {
+                name,
+                task,
+                value,
+                target,
+            } => write!(
+                f,
+                "Illegal {name} while {task}: Failed to cast {value} to {target}",
+                name = name,
+                task = task,
+                value = value,
+                target = target
+            ),
+        }
+    }
+}
+
+/// The task being attempted when the C API returns an error
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum ErrorTask {
+    /// The number of atoms was being read from a file
+    ReadNumAtoms,
+    /// A frame was being read from a file
+    Read,
+    /// A frame was being written to a file
+    Write,
+    /// A file was being flushed to disk
+    Flush,
+    /// A seek operation was being run on a file
+    Seek,
+}
+
+impl std::fmt::Display for ErrorTask {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match &self {
+            ErrorTask::ReadNumAtoms => write!(f, "reading atom number from trajectory"),
+            ErrorTask::Read => write!(f, "reading trajectory"),
+            ErrorTask::Write => write!(f, "writing trajectory"),
+            ErrorTask::Flush => write!(f, "flushing trajectory"),
+            ErrorTask::Seek => write!(f, "seeking in trajectory"),
+        }
+    }
+}
+
+/// Error codes returned from the C API
+#[derive(Debug, Clone, PartialEq, Copy)]
+pub enum ErrorCode {
+    /// No error, C API returned successfully
+    ExdrOk,
+    /// TRR file had corrupt Header
+    ExdrHeader,
+    /// Failed to read a string where expected
+    ExdrString,
+    /// Failed to read a double where expected
+    ExdrDouble,
+    /// Failed to read an int where expected
+    ExdrInt,
+    /// Failed to read a float where expected
+    ExdrFloat,
+    /// Failed to read a uint where expected
+    ExdrUint,
+    /// Failed to read compressed XTC coordinates
+    Exdr3dx,
+    /// Error encountered while closing file
+    ExdrClose,
+    /// File had incorrect "magic number" (not an XTC file)
+    ExdrMagic,
+    /// Failed to allocate memory for TRR file
+    ExdrNoMem,
+    /// End of file was reached while trying to read
+    ExdrEndOfFile,
+    /// File was not found when trying to open
+    ExdrFileNotFound,
+    /// Failed to seek within file
+    ExdrNr,
+    /// Something unexpected happened
+    UnmatchedCode(i32),
+}
+
+impl ErrorCode {
+    /// True if the error is an end of file error, false otherwise
+    pub fn is_eof(self) -> bool {
+        matches!(self, Self::ExdrEndOfFile)
+    }
+}
+
+impl From<i32> for ErrorCode {
+    fn from(code: i32) -> Self {
+        match code {
+            c_abi::xdrfile::exdrOK => Self::ExdrOk,
+            c_abi::xdrfile::exdrHEADER => Self::ExdrHeader,
+            c_abi::xdrfile::exdrSTRING => Self::ExdrString,
+            c_abi::xdrfile::exdrDOUBLE => Self::ExdrDouble,
+            c_abi::xdrfile::exdrINT => Self::ExdrInt,
+            c_abi::xdrfile::exdrFLOAT => Self::ExdrFloat,
+            c_abi::xdrfile::exdrUINT => Self::ExdrUint,
+            c_abi::xdrfile::exdr3DX => Self::Exdr3dx,
+            c_abi::xdrfile::exdrCLOSE => Self::ExdrClose,
+            c_abi::xdrfile::exdrMAGIC => Self::ExdrMagic,
+            c_abi::xdrfile::exdrNOMEM => Self::ExdrNoMem,
+            c_abi::xdrfile::exdrENDOFFILE => Self::ExdrEndOfFile,
+            c_abi::xdrfile::exdrFILENOTFOUND => Self::ExdrFileNotFound,
+            c_abi::xdrfile::exdrNR => Self::ExdrNr,
+            code => Self::UnmatchedCode(code),
+        }
+    }
+}
+
+impl std::fmt::Display for ErrorCode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Self::UnmatchedCode(i) = self {
+            write!(f, "{}", i)
+        } else {
+            write!(f, "{:?}", self)
+        }
+    }
+}
+
+/// `Result` type for errors in the `xdrfile` crate
+pub type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_is_eof() {
+        let error = Error::CApiError {
+            code: c_abi::xdrfile::exdrENDOFFILE.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = Error::CApiError {
+            code: ErrorCode::ExdrEndOfFile,
+            task: ErrorTask::Read,
+        };
+        assert!(error.is_eof());
+
+        let error = Error::CApiError {
+            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error::CApiError {
+            code: 0.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error::CApiError {
+            code: 255.into(),
+            task: ErrorTask::Read,
+        };
+        assert!(!error.is_eof());
+
+        let error = Error::CouldNotOpen {
+            path: PathBuf::from("not/a/file"),
+            mode: FileMode::Read,
+        };
+        assert!(!error.is_eof());
+    }
+
+    #[test]
+    fn test_from_correct_type() {
+        let code = 3.into();
+        let task = ErrorTask::Read;
+        let expected = Error::CApiError { code, task };
+        let err = Error::from((code, task));
+        assert_eq!(expected, err);
+
+        let path = Path::new(".");
+        let mode = FileMode::Read;
+        let expected = Error::CouldNotOpen {
+            path: path.to_path_buf(),
+            mode: mode.to_owned(),
+        };
+        let err = Error::from((path, mode));
+        assert_eq!(expected, err);
+
+        let frame = Frame::with_len(0);
+        let expected = Error::WrongSizeFrame {
+            expected: 10,
+            found: 0,
+        };
+        let err = Error::from((&frame, 10));
+        assert_eq!(expected, err);
+    }
+}

src/frame.rs

@@ -1,28 +1,24 @@
-use std::fmt;
+use std::ops::{Index, IndexMut};
 
 /// A frame represents a single step in a trajectory.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Frame {
-    /// Number of atoms in the frame
-    pub num_atoms: u32,
-
     /// Trajectory step
-    pub step: u32,
+    pub step: usize,
 
     /// Time step (usually in picoseconds)
     pub time: f32,
 
     /// 3x3 box vector
-    pub box_vector: [[f32; 3usize]; 3usize],
+    pub box_vector: [[f32; 3]; 3],
 
     /// 3D coordinates for N atoms where N is num_atoms
-    pub coords: Vec<[f32; 3usize]>,
+    pub coords: Vec<[f32; 3]>,
 }
 
 impl Default for Frame {
     fn default() -> Frame {
         Frame {
-            num_atoms: 0,
             step: 0,
             time: 0.0,
             box_vector: [[0.0; 3]; 3],
@@ -31,51 +27,59 @@ impl Default for Frame {
     }
 }
 
-impl fmt::Debug for Frame {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(
-            f,
-            "Frame {{ atoms: {}, step: {}, time: {}, \
-            box: {:?}, coords: {:?} }}",
-            self.num_atoms, self.step, self.time, self.box_vector, self.coords
-        )
-    }
-}
-
 impl Frame {
     /// Creates an empty frame with a capacity of 0
     pub fn new() -> Frame {
-        Frame {
+        Default::default()
-            ..Default::default()
-        }
     }
 
     /// Creates a frame with the given capacity
-    pub fn with_capacity(num_atoms: u32) -> Frame {
+    pub fn with_len(num_atoms: usize) -> Frame {
         Frame {
-            num_atoms,
+            coords: vec![[0.0, 0.0, 0.0]; num_atoms],
-            coords: vec![[0.0, 0.0, 0.0]; num_atoms as usize],
             ..Default::default()
         }
     }
 
     /// Filters the frame by removing all atoms not matching the given indeces.
-    pub fn filter_coords(self: &mut Frame, indeces: &[usize]) {
+    pub fn filter_coords(self: &mut Frame, indices: &[usize]) {
         self.coords = self
             .coords
             .iter()
-            .map(|elem| elem.clone())
             .enumerate()
-            .filter(|&(i, _)| indeces.contains(&i))
+            .filter(|(i, _)| indices.contains(i))
-            .map(|(_, elem)| elem)
+            .map(|(_, elem)| *elem)
             .collect();
-        self.num_atoms = self.coords.len() as u32;
     }
 
     /// Length of the frame (number of atoms)
     pub fn len(self: &Frame) -> usize {
-        self.num_atoms as usize
+        self.num_atoms()
     }
+
+    /// The number of atoms in the frame
+    pub fn num_atoms(self: &Frame) -> usize {
+        self.coords.len()
+    }
+
+    /// Resize the frame to have exactly `num_atoms` atoms, filling coords with zeros if necessary
+    pub fn resize(&mut self, num_atoms: usize) {
+        self.coords.resize(num_atoms, [0.0; 3])
+    }
+}
+
+impl Index<usize> for Frame {
+    type Output = [f32; 3];
+
+    fn index(&self, index: usize) -> &Self::Output {
+        &self.coords[index]
+    }
+}
+
+impl IndexMut<usize> for Frame {
+    fn index_mut(&mut self, index: usize) -> &mut Self::Output { {
+        &mut self.coords[index]
+    }}
 }
 
 #[cfg(test)]
@@ -84,28 +88,84 @@ mod tests {
 
     #[test]
     fn test_frame_with_capacity() {
-        let frame = Frame::with_capacity(10);
+        let frame = Frame::with_len(10);
         println!("{:?}", frame.coords);
         assert_eq!(frame.coords.len(), 10);
     }
 
     #[test]
     fn test_frame_filter_atoms() {
-        let mut frame = Frame::with_capacity(3);
+        let mut frame = Frame::with_len(3);
-        frame.coords[0] = [1.0, 2.0, 3.0];
+        frame[0] = [1.0, 2.0, 3.0];
-        frame.coords[1] = [4.0, 5.0, 6.0];
+        frame[1] = [4.0, 5.0, 6.0];
-        frame.coords[2] = [7.0, 8.0, 9.0];
+        frame[2] = [7.0, 8.0, 9.0];
         let filter: Vec<usize> = vec![1, 2];
         let mut frame_new = frame.clone();
         frame_new.filter_coords(&filter);
-        assert!(frame_new.num_atoms as usize == filter.len());
+        assert!(frame_new.len() == filter.len());
-        assert!(frame_new.coords[0] == frame.coords[1]);
+        assert!(frame_new.coords[0] == frame[1]);
-        assert!(frame_new.coords[1] == frame.coords[2]);
+        assert!(frame_new.coords[1] == frame[2]);
     }
 
     #[test]
     fn test_frame_len() {
-        let frame = Frame::with_capacity(10);
+        let frame = Frame::with_len(10);
         assert_eq!(frame.len(), 10);
     }
+
+    #[test]
+    fn test_filter_coords() {
+        let mut frame = Frame {
+            step: 0,
+            time: 0.0,
+            box_vector: [[0.0; 3]; 3],
+            coords: vec![[0.0; 3], [1.0; 3], [2.0; 3]]
+        };
+
+        frame.filter_coords(&[1]);
+        for i in 0..3 {
+            assert_approx_eq!(frame[0][i], 1.0);
+        }
+    }
+
+    #[test]
+    #[allow(unused_mut)]
+    fn test_index() {
+        // test Index
+        let frame = Frame {
+            step: 0,
+            time: 0.0,
+            box_vector: [[0.0; 3]; 3],
+            coords: vec![[0.0; 3], [1.0; 3], [2.0; 3]]
+        };
+        for i in 0..frame.len() {
+            for j in 0..3 {
+                assert_approx_eq!(frame[i][j], frame[i][j]);
+            }
+        }
+
+        // test IndexMut
+        let mut frame = Frame {
+            step: 0,
+            time: 0.0,
+            box_vector: [[0.0; 3]; 3],
+            coords: vec![[0.0; 3], [1.0; 3], [2.0; 3]]
+        };
+        for i in 0..frame.len() {
+            for j in 0..3 {
+                assert_approx_eq!(frame[i][j], frame.coords[i][j]);
+            }
+        }
+
+        frame[0] = [123.0; 3];
+        for i in 0..frame.len() {
+            for j in 0..3 {
+                assert_approx_eq!(frame[i][j], frame.coords[i][j]);
+                if i == 0 {
+                    assert_approx_eq!(frame[i][j], 123.0);    
+                }
+            }
+        }
+
+    }
 }

src/iterator.rs

@@ -1,116 +1,89 @@
-use crate::c_abi::xdrfile::exdrENDOFFILE;
 use crate::*;
 use std::rc::Rc;
 
-impl IntoIterator for XTCTrajectory {
+fn into_iter_inner<T: Trajectory>(mut traj: T) -> TrajectoryIterator<T> {
-    type Item = Result<Rc<Frame>>;
+    let num_atoms = traj.get_num_atoms();
-    type IntoIter = XTCTrajectoryIterator;
+    let frame = match &num_atoms {
-
+        Ok(num_atoms) => Frame::with_len(*num_atoms),
-    fn into_iter(mut self) -> Self::IntoIter {
+        Err(_) => Frame::new(),
-        // TODO this should be handled without the requirement of unwrap
+    };
-        let num_atoms = self.get_num_atoms().unwrap();
+    TrajectoryIterator {
-        XTCTrajectoryIterator {
+        trajectory: traj,
-            trajectory: self,
+        item: Rc::new(frame),
-            item: Rc::new(Frame::with_capacity(num_atoms)),
+        has_error: false,
-            has_error: false,
-        }
     }
 }
 
-/*
+impl IntoIterator for XTCTrajectory {
-Iterator for trajectories. This iterator yields a Result<Frame, Error>
-for each frame in the trajectory file and stops with yielding None once the
-trajectory is finished. Also yields None after the first occurence of an error
-*/
-pub struct XTCTrajectoryIterator {
-    trajectory: XTCTrajectory,
-    item: Rc<Frame>,
-    has_error: bool,
-}
-
-impl Iterator for XTCTrajectoryIterator {
     type Item = Result<Rc<Frame>>;
+    type IntoIter = TrajectoryIterator<XTCTrajectory>;
 
-    fn next(&mut self) -> Option<Self::Item> {
+    fn into_iter(self) -> Self::IntoIter {
-        // Reuse old frame
+        into_iter_inner(self)
-        if self.has_error {
-            return None;
-        }
-
-        let item: &mut Frame = match Rc::get_mut(&mut self.item) {
-            Some(item) => item,
-            None => {
-                // caller kept frame. Create new one
-                self.item = Rc::new(Frame::with_capacity(self.item.num_atoms));
-                Rc::get_mut(&mut self.item).unwrap()
-            }
-        };
-        match self.trajectory.read(item) {
-            Ok(()) => Some(Ok(Rc::clone(&self.item))),
-            Err(msg) => {
-                if msg.to_string().contains(&exdrENDOFFILE.to_string()) {
-                    None
-                } else {
-                    self.has_error = true;
-                    Some(Err(msg))
-                }
-            }
-        }
     }
 }
 
 impl IntoIterator for TRRTrajectory {
     type Item = Result<Rc<Frame>>;
-    type IntoIter = TRRTrajectoryIterator;
+    type IntoIter = TrajectoryIterator<TRRTrajectory>;
 
-    fn into_iter(mut self) -> Self::IntoIter {
+    fn into_iter(self) -> Self::IntoIter {
-        // TODO this should be handled without the requirement of unwrap
+        into_iter_inner(self)
-        let num_atoms = self.get_num_atoms().unwrap();
-        TRRTrajectoryIterator {
-            trajectory: self,
-            item: Rc::new(Frame::with_capacity(num_atoms)),
-            has_error: false,
-        }
     }
 }
 
-/*
+/// Iterator for trajectories.
-Iterator for trajectories. This iterator yields a Result<Frame, Error>
+/// This iterator yields a Result<Frame, Error> for each frame in the
-for each frame in the trajectory file and stops with yielding None once the
+/// trajectory file and stops with yielding None once the trajectory is
-trajectory is finished. Also yields None after the first occurence of an error
+/// EOF. Also yields None after the first occurrence of an error
-*/
+pub struct TrajectoryIterator<T> {
-pub struct TRRTrajectoryIterator {
+    trajectory: T,
-    trajectory: TRRTrajectory,
     item: Rc<Frame>,
     has_error: bool,
 }
 
-impl Iterator for TRRTrajectoryIterator {
+impl<T: Trajectory> TrajectoryIterator<T> {
-    type Item = Result<Rc<Frame>>;
+    /// Inner function for `next()`  to seperate error handling from iteration logic
+    fn next_inner(&mut self) -> <Self as Iterator>::Item {
+        // If we couldn't read the number of frames when we called into_iter, return that error now
+        // It's OK to do this every frame because the result is cached by Trajectory
+        let num_atoms = match &self.trajectory.get_num_atoms() {
+            &Ok(n) => n,
+            Err(e) => return Err(Error::CouldNotCheckNAtoms(Box::new(e.clone()))),
+        };
 
-    fn next(&mut self) -> Option<Self::Item> {
         // Reuse old frame
-        if self.has_error {
-            return None;
-        }
-
         let item: &mut Frame = match Rc::get_mut(&mut self.item) {
             Some(item) => item,
             None => {
                 // caller kept frame. Create new one
-                self.item = Rc::new(Frame::with_capacity(self.item.num_atoms));
+                self.item = Rc::new(Frame::with_len(num_atoms as usize));
-                Rc::get_mut(&mut self.item).unwrap()
+                Rc::get_mut(&mut self.item).expect("Could not get mutable access to new Rc")
             }
         };
-        match self.trajectory.read(item) {
+
-            Ok(()) => Some(Ok(Rc::clone(&self.item))),
+        self.trajectory.read(item)?;
-            Err(msg) => {
+        Ok(Rc::clone(&self.item))
-                if msg.to_string().contains(&exdrENDOFFILE.to_string()) {
+    }
-                    None
+}
-                } else {
+
-                    self.has_error = true;
+impl<T> Iterator for TrajectoryIterator<T>
-                    Some(Err(msg))
+where
-                }
+    T: Trajectory,
+{
+    type Item = Result<Rc<Frame>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.has_error {
+            return None;
+        }
+
+        match self.next_inner() {
+            Ok(item) => Some(Ok(item)),
+            Err(e) if e.is_eof() => None,
+            Err(e) => {
+                self.has_error = true;
+                Some(Err(e))
             }
         }
     }
@@ -121,20 +94,24 @@ mod tests {
     use super::*;
 
     #[test]
-    pub fn test_xtc_trajectory_iterator() {
+    pub fn test_xtc_trajectory_iterator() -> Result<()> {
-        let traj = XTCTrajectory::open(Path::new("tests/1l2y.xtc"), FileMode::Read).unwrap();
+        let traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
-        let frames: Vec<Rc<Frame>> = traj.into_iter().filter_map(Result::ok).collect();
+        let frames: Result<Vec<Rc<Frame>>> = traj.into_iter().collect();
+        let frames = frames?;
         assert!(frames.len() == 38);
         assert!(frames[0].step == 1, frames[0].step);
         assert!(frames[37].step == 38);
+        Ok(())
     }
 
     #[test]
-    pub fn test_trr_trajectory_iterator() {
+    pub fn test_trr_trajectory_iterator() -> Result<()> {
-        let traj = TRRTrajectory::open(Path::new("tests/1l2y.trr"), FileMode::Read).unwrap();
+        let traj = TRRTrajectory::open_read("tests/1l2y.trr")?;
-        let frames: Vec<Rc<Frame>> = traj.into_iter().filter_map(Result::ok).collect();
+        let frames: Result<Vec<Rc<Frame>>> = traj.into_iter().collect();
+        let frames = frames?;
         assert!(frames.len() == 38);
         assert!(frames[0].step == 1, frames[0].step);
         assert!(frames[37].step == 38);
+        Ok(())
     }
 }

tests/integration.rs

@@ -1,31 +1,28 @@
 #[cfg(test)]
 mod integration {
 
-    use std::path::Path;
     use std::rc::Rc;
     use xdrfile::*;
 
     #[test]
-    fn test_use_library() {
+    fn test_use_library() -> Result<()> {
-        let path = Path::new("tests/1l2y.xtc");
+        let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+        let num_atoms = trj.get_num_atoms()?;
+        let mut frame = Frame::with_len(num_atoms);
 
-        let mut trj = XTCTrajectory::open(path, FileMode::Read).unwrap();
+        trj.read(&mut frame)?;
-        let num_atoms = trj.get_num_atoms().unwrap();
+        trj.read(&mut frame)?;
-        let mut frame = Frame::with_capacity(num_atoms);
-
-        trj.read(&mut frame).unwrap();
-        trj.read(&mut frame).unwrap();
         assert_eq!(frame.step, 2);
+        Ok(())
     }
 
     #[test]
-    fn test_use_library_iterator() {
+    fn test_use_library_iterator() -> Result<()> {
-        let path = Path::new("tests/1l2y.xtc");
+        let trj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
-
+        let frames: Result<Vec<Rc<Frame>>> = trj.into_iter().collect();
-        let trj = XTCTrajectory::open(path, FileMode::Read).unwrap();
+        for (idx, frame) in frames?.iter().enumerate() {
-        let frames: Vec<Rc<Frame>> = trj.into_iter().filter_map(Result::ok).collect();
+            assert_eq!(frame.step, idx + 1);
-        for (idx, frame) in frames.iter().enumerate() {
-            assert_eq!(frame.step as usize, idx + 1);
         }
+        Ok(())
     }
 }