From 03b03f1f173c24add88513ecb05acd373b4ac020 Mon Sep 17 00:00:00 2001 From: "J. Sebastian Paez" Date: Tue, 1 Sep 2026 11:16:12 -0700 Subject: [PATCH 1/2] test: added failing test --- mzannotate/tests/mzspeclib.rs | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/mzannotate/tests/mzspeclib.rs b/mzannotate/tests/mzspeclib.rs index b7999c96..4996c098 100644 --- a/mzannotate/tests/mzspeclib.rs +++ b/mzannotate/tests/mzspeclib.rs @@ -133,3 +133,39 @@ fn read_all_files() { // "Some interpretation attributes unused" // ); } + +#[test] +fn a_second_and_a_minute_declaration_of_one_instant_agree() { + /// The same instant, declared twice: 1719.50946 s is 28.658491 min. + const LIBRARY: &str = " +MS:1003186|library format version=1.0 + +[1]MS:1000894|retention time=1719.509460 +[1]UO:0000000|unit=UO:0000010|second + + + +[1]MS:1000894|retention time=28.658491 +[1]UO:0000000|unit=UO:0000031|minute + +"; + + let spectra: Vec<_> = MzSpecLibTextParser::open( + LIBRARY.as_bytes(), + None, + &mzcore::ontology::STATIC_ONTOLOGIES, + ) + .unwrap() + .map(|s| s.unwrap()) + .collect(); + + let times: Vec = spectra + .iter() + .map(|x| x.description.acquisition.scans[0].start_time) + .collect(); + + assert_eq!(times[0], times[1], "1719.50946 s is 28.658491 min"); + // mzdata documents start_time as minutes (mzdata::/src/spectrum/scan_properties.rs:138). The + // tolerance is for the to_f32 the parser applies on the way in. + assert!((times[0] - 28.658_491).abs() < 1e-4, "got {}", times[0]); +} From 1f6733aa737405e6143b80b91673b34b49e7aea3 Mon Sep 17 00:00:00 2001 From: "J. Sebastian Paez" Date: Tue, 1 Sep 2026 12:02:01 -0700 Subject: [PATCH 2/2] fix: make every mzspeclib return minute RTs --- .../src/mzspeclib/spectrum_description.rs | 69 ++++++++++++------- mzannotate/tests/mzspeclib.rs | 33 ++++++++- 2 files changed, 73 insertions(+), 29 deletions(-) diff --git a/mzannotate/src/mzspeclib/spectrum_description.rs b/mzannotate/src/mzspeclib/spectrum_description.rs index 55fcb2ea..b03b4d49 100644 --- a/mzannotate/src/mzspeclib/spectrum_description.rs +++ b/mzannotate/src/mzspeclib/spectrum_description.rs @@ -1,5 +1,5 @@ //! Handle converting [`Attribute`]s to and from [`SpectrumDescription`] -use std::borrow::Cow; +use std::{borrow::Cow, hint::black_box}; use context_error::{BoxedError, Context, CreateError}; use mzcv::{AccessionCode, Curie, Term, curie, term}; @@ -287,39 +287,49 @@ pub(crate) fn populate_spectrum_description_from_attributes<'a>( if let Some((Attribute { value, .. }, context)) = group.iter().find(|a| a.0.name.accession == curie!(MS:1000894)) - && group.len() == 2 + && group.len() >= 2 { - description.acquisition.scans[0].start_time = - f64::from(value.scalar().to_f32().map_err(|v| { - BoxedError::new( - MzSpecLibErrorKind::Attribute, - "Invalid attribute", - v.to_string(), - context.clone(), - ) - })?) / match unit.ok_or_else(|| { - BoxedError::new( - MzSpecLibErrorKind::MissingUnit, - "Invalid attribute", - "MS:1000894|retention time needs a unit", - context.clone(), - ) - })? { - Unit::Minute => 60.0, - _ => 1.0, // Assume seconds for anything else - }; + let enumerator = f64::from(value.scalar().to_f32().map_err(|v| { + BoxedError::new( + MzSpecLibErrorKind::Attribute, + "Invalid attribute", + v.to_string(), + context.clone(), + ) + })?); + let divisor = match unit.ok_or_else(|| { + BoxedError::new( + MzSpecLibErrorKind::MissingUnit, + "Invalid attribute", + "MS:1000894|retention time needs a unit", + context.clone(), + ) + })? { + Unit::Minute => 1.0, + Unit::Millisecond => 60.0 * (1000.0), + Unit::Second => 60.0, + _ => { + black_box(()); // We should probably warn here + 1.0 // Assume minutes for anything else + } + }; + + // Start time is meant to be represented in "minutes" + description.acquisition.scans[0].start_time = enumerator / divisor; } else if let Some((Attribute { value, .. }, context)) = group.iter().find(|a| a.0.name.accession == curie!(MS:1000896)) - && group.len() == 2 + && group.len() >= 2 { - let rt = f64::from(value.scalar().to_f32().map_err(|v| { + let enumerator = f64::from(value.scalar().to_f32().map_err(|v| { BoxedError::new( MzSpecLibErrorKind::Attribute, "Invalid attribute", v.to_string(), context.clone(), ) - })?) / match unit.ok_or_else(|| { + })?); + + let denominator = match unit.ok_or_else(|| { BoxedError::new( MzSpecLibErrorKind::MissingUnit, "Invalid attribute", @@ -327,9 +337,16 @@ pub(crate) fn populate_spectrum_description_from_attributes<'a>( context.clone(), ) })? { - Unit::Minute => 60.0, - _ => 1.0, // Assume seconds for anything else + Unit::Minute => 1.0, + Unit::Millisecond => 60.0 * (1000.0), + Unit::Second => 60.0, + _ => { + black_box(()); // We should probably warn here + 1.0 // Assume minutes for anything else + } }; + let rt = enumerator / denominator; + // This is normalised time so if normal time is already set ignore this param if description.acquisition.scans[0].start_time == 0.0 { description.acquisition.scans[0].start_time = rt; diff --git a/mzannotate/tests/mzspeclib.rs b/mzannotate/tests/mzspeclib.rs index 4996c098..795a616d 100644 --- a/mzannotate/tests/mzspeclib.rs +++ b/mzannotate/tests/mzspeclib.rs @@ -136,7 +136,7 @@ fn read_all_files() { #[test] fn a_second_and_a_minute_declaration_of_one_instant_agree() { - /// The same instant, declared twice: 1719.50946 s is 28.658491 min. + /// The same instant: 1719.50946 s == 28.658491 min. const LIBRARY: &str = " MS:1003186|library format version=1.0 @@ -148,6 +148,24 @@ MS:1003186|library format version=1.0 [1]MS:1000894|retention time=28.658491 [1]UO:0000000|unit=UO:0000031|minute + + +[1]MS:1000894|retention time=1719509.460 +[1]UO:0000000|unit=UO:0000028|millisecond + + +# JSP: This is silently replaced to 0.0 +# which I am not sure is the correct behavior +# +# [1]MS:1000894|retention time=28.658491 +# + + +[1]MS:1000894|retention time=1719.509460 +[1]UO:0000000|unit=UO:0000010|second +[1]MS:1003174|attribute maximum=1800.0 +[1]MS:1003175|attribute minimum=1600.0 + "; let spectra: Vec<_> = MzSpecLibTextParser::open( @@ -164,8 +182,17 @@ MS:1003186|library format version=1.0 .map(|x| x.description.acquisition.scans[0].start_time) .collect(); - assert_eq!(times[0], times[1], "1719.50946 s is 28.658491 min"); + fn assert_approx_eq(a: f64, b: f64, msg: &str) { + if (a - b).abs() > 1e-4 { + panic!("no aprox eq {a} {b}; {msg}"); + } + } + + assert_approx_eq(times[0], times[1], "1719.50946 s is 28.658491 min"); + assert_approx_eq(times[0], times[2], "1719.50946 s is 1719509.46 ms"); // mzdata documents start_time as minutes (mzdata::/src/spectrum/scan_properties.rs:138). The // tolerance is for the to_f32 the parser applies on the way in. - assert!((times[0] - 28.658_491).abs() < 1e-4, "got {}", times[0]); + for (idx, x) in times.iter().enumerate() { + assert_approx_eq(*x, 28.658_491, &format!("got {x} at idx {idx}")); + } }