From 734048d5726f21ee91d2f46f65139f62425d783d Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Tue, 10 Nov 2020 20:38:24 +1100 Subject: [PATCH 1/3] Refactor frame to have desired semantics --- README.md | 6 +++--- src/errors.rs | 2 +- src/frame.rs | 44 +++++++++++++++----------------------------- src/iterator.rs | 6 +++--- src/lib.rs | 29 +++++++++++++---------------- tests/integration.rs | 2 +- 6 files changed, 36 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index de536c0..4cf3b7a 100644 --- a/README.md +++ b/README.md @@ -17,17 +17,17 @@ fn main() -> Result<()> { let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; // find number of atoms in the file - let num_atoms = trj.get_num_atoms()?; + let num_atoms = trj.get_num_atoms()? as usize; // a frame object is used to get to read or write from a trajectory // 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 trj.read(&mut frame)?; assert_eq!(frame.step, 1); - assert_eq!(frame.num_atoms, num_atoms); + assert_eq!(frame.len(), num_atoms); let first_atom_coords = frame.coords[0]; assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); diff --git a/src/errors.rs b/src/errors.rs index 482b036..84d5e39 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -304,7 +304,7 @@ mod tests { let err = Error::from((path, mode)); assert_eq!(expected, err); - let frame = Frame::with_capacity(0); + let frame = Frame::with_len(0); let expected = Error::WrongSizeFrame { expected: 10, found: 0, diff --git a/src/frame.rs b/src/frame.rs index 805f8e7..14f3ac8 100644 --- a/src/frame.rs +++ b/src/frame.rs @@ -1,11 +1,6 @@ -use std::fmt; - /// 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, @@ -13,16 +8,15 @@ pub struct Frame { 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,17 +25,6 @@ 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 { @@ -51,10 +34,9 @@ impl Frame { } /// 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 as usize], + coords: vec![[0.0, 0.0, 0.0]; num_atoms], ..Default::default() } } @@ -69,12 +51,16 @@ impl Frame { .filter(|&(i, _)| indeces.contains(&i)) .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.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]) } } @@ -84,28 +70,28 @@ 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.coords[1] = [4.0, 5.0, 6.0]; frame.coords[2] = [7.0, 8.0, 9.0]; let filter: Vec = 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[1] == frame.coords[2]); } #[test] fn test_frame_len() { - let frame = Frame::with_capacity(10); + let frame = Frame::with_len(10); assert_eq!(frame.len(), 10); } } diff --git a/src/iterator.rs b/src/iterator.rs index 48ee466..4b1e00f 100644 --- a/src/iterator.rs +++ b/src/iterator.rs @@ -2,9 +2,9 @@ use crate::*; use std::rc::Rc; fn into_iter_inner(mut traj: T) -> TrajectoryIterator { - let num_atoms = traj.get_num_atoms(); + let num_atoms = traj.get_num_atoms().map(|n| n as usize); let frame = match &num_atoms { - Ok(num_atoms) => Frame::with_capacity(*num_atoms), + Ok(num_atoms) => Frame::with_len(*num_atoms), Err(_) => Frame::new(), }; TrajectoryIterator { @@ -58,7 +58,7 @@ impl TrajectoryIterator { Some(item) => item, None => { // caller kept frame. Create new one - self.item = Rc::new(Frame::with_capacity(num_atoms)); + self.item = Rc::new(Frame::with_len(num_atoms as usize)); Rc::get_mut(&mut self.item).expect("Could not get mutable access to new Rc") } }; diff --git a/src/lib.rs b/src/lib.rs index c4803cc..f80e1bc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,17 +14,17 @@ //! let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; //! //! // find number of atoms in the file -//! let num_atoms = trj.get_num_atoms()?; +//! let num_atoms = trj.get_num_atoms()? as usize; //! //! // a frame object is used to get to read or write from a trajectory //! // 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 //! trj.read(&mut frame)?; //! //! assert_eq!(frame.step, 1); -//! assert_eq!(frame.num_atoms, num_atoms); +//! assert_eq!(frame.len(), num_atoms); //! //! let first_atom_coords = frame.coords[0]; //! assert_eq!(first_atom_coords, [-0.8901, 0.4127, -0.055499997]); @@ -278,7 +278,7 @@ impl Trajectory for XTCTrajectory { unsafe { let code = xdrfile_xtc::write_xtc( self.handle.xdrfile, - frame.num_atoms as i32, + frame.len() as i32, frame.step as i32, frame.time, frame.box_vector.as_ptr() as *mut [[f32; 3]; 3], @@ -413,7 +413,7 @@ impl Trajectory for TRRTrajectory { unsafe { let code = xdrfile_trr::write_trr( self.handle.xdrfile, - frame.num_atoms as i32, + frame.len() as i32, frame.step as i32, frame.time, 0.0, @@ -492,7 +492,6 @@ mod tests { let natoms: u32 = 2; let frame = Frame { - num_atoms: natoms, step: 5, time: 2.0, box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], @@ -506,7 +505,7 @@ mod tests { } f.flush()?; - let mut new_frame = Frame::with_capacity(natoms); + let mut new_frame = Frame::with_len(natoms as usize); let mut f = XTCTrajectory::open_read(tmp_path)?; let num_atoms = f.get_num_atoms()?; assert_eq!(num_atoms, natoms); @@ -517,7 +516,7 @@ mod tests { Ok(()) => {} } - assert_eq!(new_frame.num_atoms, frame.num_atoms); + assert_eq!(new_frame.len(), frame.len()); assert_eq!(new_frame.step, frame.step); assert_approx_eq!(new_frame.time, frame.time); assert_eq!(new_frame.box_vector, frame.box_vector); @@ -532,7 +531,6 @@ mod tests { let natoms: u32 = 2; let frame = Frame { - num_atoms: natoms, step: 5, time: 2.0, box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], @@ -546,7 +544,7 @@ mod tests { } f.flush()?; - let mut new_frame = Frame::with_capacity(natoms); + let mut new_frame = Frame::with_len(natoms as usize); let mut f = TRRTrajectory::open_read(tmp_path)?; // let num_atoms = f.get_num_atoms()?; // assert_eq!(num_atoms, natoms); @@ -557,7 +555,7 @@ mod tests { Ok(()) => {} } - assert_eq!(new_frame.num_atoms, frame.num_atoms); + assert_eq!(new_frame.len(), frame.len()); assert_eq!(new_frame.step, frame.step); assert_eq!(new_frame.time, frame.time); assert_eq!(new_frame.box_vector, frame.box_vector); @@ -569,7 +567,7 @@ mod tests { pub fn test_manual_loop() -> Result<(), Box> { let mut xtc_frames = Vec::new(); let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?; - let mut frame = Frame::with_capacity(xtc_traj.get_num_atoms()?); + let mut frame = Frame::with_len(xtc_traj.get_num_atoms()? as usize); while let Ok(()) = xtc_traj.read(&mut frame) { xtc_frames.push(frame.clone()); @@ -583,7 +581,7 @@ mod tests { } for (xtc, trr) in xtc_frames.into_iter().zip(trr_frames) { - assert_eq!(xtc.num_atoms, trr.num_atoms); + assert_eq!(xtc.len(), trr.len()); assert_eq!(xtc.step, trr.step); assert_eq!(xtc.time, trr.time); assert_eq!(xtc.box_vector, trr.box_vector); @@ -732,7 +730,7 @@ mod tests { #[test] fn test_err_could_not_read() -> Result<()> { let file_name = "README.md"; // not a trajectory - let mut frame = Frame::with_capacity(1); + let mut frame = Frame::with_len(1); let mut trr = TRRTrajectory::open_read(file_name)?; if let Err(e) = trr.read(&mut frame) { assert_eq!(Some(ErrorCode::ExdrMagic), e.code()); @@ -749,7 +747,6 @@ mod tests { let natoms: u32 = 2; let frame = Frame { - num_atoms: natoms, step: 5, time: 2.0, box_vector: [[1.0, 2.0, 3.0], [2.0, 1.0, 3.0], [3.0, 2.0, 1.0]], @@ -759,7 +756,7 @@ mod tests { f.write(&frame)?; f.flush()?; - let mut new_frame = Frame::with_capacity(natoms); + let mut new_frame = Frame::with_len(natoms as usize); let mut f = XTCTrajectory::open_read(tmp_path)?; f.read(&mut new_frame)?; diff --git a/tests/integration.rs b/tests/integration.rs index f084289..c7fb158 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -8,7 +8,7 @@ mod integration { fn test_use_library() -> Result<()> { let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; let num_atoms = trj.get_num_atoms()?; - let mut frame = Frame::with_capacity(num_atoms); + let mut frame = Frame::with_len(num_atoms as usize); trj.read(&mut frame)?; trj.read(&mut frame)?; From 3c36d3040eeeea815528a6b021be77eb2c2b9343 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Tue, 10 Nov 2020 20:45:38 +1100 Subject: [PATCH 2/3] Removed trivial casts and changed frame.step to usize --- README.md | 4 ++-- src/c_abi/xdrfile_trr.rs | 4 ++-- src/c_abi/xdrfile_xtc.rs | 2 +- src/frame.rs | 2 +- src/iterator.rs | 2 +- src/lib.rs | 41 ++++++++++++++++++++-------------------- 6 files changed, 28 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 4cf3b7a..d209c78 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ fn main() -> Result<()> { let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; // find number of atoms in the file - let num_atoms = trj.get_num_atoms()? as usize; + let num_atoms = trj.get_num_atoms()?; // a frame object is used to get to read or write from a trajectory // without instantiating data arrays for every step @@ -53,7 +53,7 @@ fn main() -> Result<()> { for (idx, result) in trj.into_iter().enumerate() { let frame = result?; println!("{}", frame.time); - assert_eq!(idx+1, frame.step as usize); + assert_eq!(idx+1, frame.step); } Ok(()) } diff --git a/src/c_abi/xdrfile_trr.rs b/src/c_abi/xdrfile_trr.rs index 751b356..a7eae5b 100644 --- a/src/c_abi/xdrfile_trr.rs +++ b/src/c_abi/xdrfile_trr.rs @@ -52,7 +52,7 @@ mod tests { 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(()) @@ -64,7 +64,7 @@ mod tests { 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!(nframes == 38, "{:?}", nframes); diff --git a/src/c_abi/xdrfile_xtc.rs b/src/c_abi/xdrfile_xtc.rs index 507904f..ab8ba8b 100644 --- a/src/c_abi/xdrfile_xtc.rs +++ b/src/c_abi/xdrfile_xtc.rs @@ -60,7 +60,7 @@ mod tests { 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!(nframes == 38, "{:?}", nframes); diff --git a/src/frame.rs b/src/frame.rs index 14f3ac8..6cabb69 100644 --- a/src/frame.rs +++ b/src/frame.rs @@ -2,7 +2,7 @@ #[derive(Clone, Debug)] pub struct Frame { /// Trajectory step - pub step: u32, + pub step: usize, /// Time step (usually in picoseconds) pub time: f32, diff --git a/src/iterator.rs b/src/iterator.rs index 4b1e00f..5887ece 100644 --- a/src/iterator.rs +++ b/src/iterator.rs @@ -2,7 +2,7 @@ use crate::*; use std::rc::Rc; fn into_iter_inner(mut traj: T) -> TrajectoryIterator { - let num_atoms = traj.get_num_atoms().map(|n| n as usize); + let num_atoms = traj.get_num_atoms(); let frame = match &num_atoms { Ok(num_atoms) => Frame::with_len(*num_atoms), Err(_) => Frame::new(), diff --git a/src/lib.rs b/src/lib.rs index f80e1bc..8e4fb4f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,7 +14,7 @@ //! let mut trj = XTCTrajectory::open_read("tests/1l2y.xtc")?; //! //! // find number of atoms in the file -//! let num_atoms = trj.get_num_atoms()? as usize; +//! let num_atoms = trj.get_num_atoms()?; //! //! // a frame object is used to get to read or write from a trajectory //! // without instantiating data arrays for every step @@ -50,7 +50,7 @@ //! for (idx, result) in trj.into_iter().enumerate() { //! let frame = result?; //! println!("{}", frame.time); -//! assert_eq!(idx+1, frame.step as usize); +//! assert_eq!(idx+1, frame.step); //! } //! Ok(()) //! } @@ -205,14 +205,14 @@ pub trait Trajectory { fn flush(&mut self) -> Result<()>; /// Get the number of atoms from the give trajectory - fn get_num_atoms(&mut self) -> Result; + fn get_num_atoms(&mut self) -> Result; } /// Read/Write XTC Trajectories pub struct XTCTrajectory { handle: XDRFile, precision: Cell, // internal mutability required for read method - num_atoms: Lazy>, + num_atoms: Lazy>, } impl XTCTrajectory { @@ -245,9 +245,9 @@ impl Trajectory for XTCTrajectory { fn read(&mut self, frame: &mut Frame) -> Result<()> { let mut step: i32 = 0; - let num_atoms = - self.get_num_atoms() - .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize; + let num_atoms = self + .get_num_atoms() + .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))?; if num_atoms != frame.coords.len() { Err((&*frame, num_atoms))?; } @@ -265,7 +265,7 @@ impl Trajectory for XTCTrajectory { frame.coords.as_mut_ptr(), &mut self.precision.get(), ) as u32; - frame.step = step as u32; + frame.step = step as usize; if let Some(err) = check_code(code, ErrorTask::Read) { Err(err) } else { @@ -304,7 +304,7 @@ impl Trajectory for XTCTrajectory { } } - fn get_num_atoms(&mut self) -> Result { + fn get_num_atoms(&mut self) -> Result { self.num_atoms .get_or_create(|| { let mut num_atoms: i32 = 0; @@ -320,7 +320,7 @@ impl Trajectory for XTCTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(num_atoms as u32) + Ok(num_atoms as usize) } } }) @@ -344,7 +344,7 @@ impl io::Seek for XTCTrajectory { /// Read/Write TRR Trajectories pub struct TRRTrajectory { handle: XDRFile, - num_atoms: Lazy>, + num_atoms: Lazy>, } impl TRRTrajectory { @@ -377,9 +377,9 @@ impl Trajectory for TRRTrajectory { let mut step: i32 = 0; let mut lambda: f32 = 0.0; - let num_atoms = - self.get_num_atoms() - .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))? as usize; + let num_atoms = self + .get_num_atoms() + .map_err(|e| Error::CouldNotCheckNAtoms(Box::new(e)))?; if num_atoms != frame.coords.len() { Err((&*frame, num_atoms))?; } @@ -400,7 +400,8 @@ impl Trajectory for TRRTrajectory { std::ptr::null_mut(), std::ptr::null_mut(), ) as u32; - frame.step = step as u32; + + frame.step = step as usize; if let Some(err) = check_code(code, ErrorTask::Read) { Err(err) } else { @@ -441,7 +442,7 @@ impl Trajectory for TRRTrajectory { } } - fn get_num_atoms(&mut self) -> Result { + fn get_num_atoms(&mut self) -> Result { self.num_atoms .get_or_create(|| { let mut num_atoms: i32 = 0; @@ -456,7 +457,7 @@ impl Trajectory for TRRTrajectory { if let Some(err) = check_code(code, ErrorTask::ReadNumAtoms) { Err(err) } else { - Ok(num_atoms as u32) + Ok(num_atoms as usize) } } }) @@ -490,7 +491,7 @@ mod tests { let tempfile = NamedTempFile::new().expect("Could not create temporary file"); let tmp_path = tempfile.path(); - let natoms: u32 = 2; + let natoms = 2; let frame = Frame { step: 5, time: 2.0, @@ -505,7 +506,7 @@ mod tests { } f.flush()?; - let mut new_frame = Frame::with_len(natoms as usize); + let mut new_frame = Frame::with_len(natoms); let mut f = XTCTrajectory::open_read(tmp_path)?; let num_atoms = f.get_num_atoms()?; assert_eq!(num_atoms, natoms); @@ -567,7 +568,7 @@ mod tests { pub fn test_manual_loop() -> Result<(), Box> { let mut xtc_frames = Vec::new(); let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?; - let mut frame = Frame::with_len(xtc_traj.get_num_atoms()? as usize); + let mut frame = Frame::with_len(xtc_traj.get_num_atoms()?); while let Ok(()) = xtc_traj.read(&mut frame) { xtc_frames.push(frame.clone()); From bbc0250af4b0b1fc2429a0bf5ef3e8791f90ca42 Mon Sep 17 00:00:00 2001 From: daniel Date: Fri, 13 Nov 2020 12:41:17 +0100 Subject: [PATCH 3/3] frame semantics for seek/tell and benchs --- benches/benchmarks.rs | 1 - src/lib.rs | 12 +++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/benches/benchmarks.rs b/benches/benchmarks.rs index d83fbb2..5668cf0 100644 --- a/benches/benchmarks.rs +++ b/benches/benchmarks.rs @@ -11,7 +11,6 @@ fn gen_test_traj(num_atoms: usize, num_frames: usize) -> Result { let mut f = XTCTrajectory::open_write(&tmp_path)?; let mut frame = Frame { - num_atoms: num_atoms as u32, 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]], diff --git a/src/lib.rs b/src/lib.rs index 8e4fb4f..c2cdac2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -634,9 +634,8 @@ mod tests { let tempfile = NamedTempFile::new()?; let tmp_path = tempfile.path(); - let natoms: u32 = 2; + let natoms: usize = 2; let frame = Frame { - num_atoms: natoms, step: 5, time: 2.0, box_vector: [[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]], @@ -648,7 +647,7 @@ mod tests { assert_eq!(f.tell(), 144); f.flush()?; - let mut new_frame = Frame::with_capacity(natoms); + let mut new_frame = Frame::with_len(natoms); let mut f = TRRTrajectory::open_read(tmp_path)?; assert_eq!(f.tell(), 0); @@ -663,9 +662,8 @@ mod tests { let tempfile = NamedTempFile::new()?; let tmp_path = tempfile.path(); - let natoms: u32 = 2; + let natoms: usize = 2; let mut frame = Frame { - num_atoms: natoms, step: 0, time: 0.0, box_vector: [[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]], @@ -680,7 +678,7 @@ mod tests { let after_second_frame = f.tell(); f.flush()?; - let mut new_frame = Frame::with_capacity(natoms); + let mut new_frame = Frame::with_len(natoms); let mut f = TRRTrajectory::open_read(tmp_path)?; let pos = f.seek(std::io::SeekFrom::Current(144))?; assert_eq!(pos, after_first_frame); @@ -688,7 +686,7 @@ mod tests { f.read(&mut new_frame)?; assert_eq!(f.tell(), after_second_frame); - assert_eq!(new_frame.num_atoms, frame.num_atoms); + assert_eq!(new_frame.len(), frame.len()); assert_eq!(new_frame.step, frame.step); assert_eq!(new_frame.time, frame.time); assert_eq!(new_frame.box_vector, frame.box_vector);