From 970ecdc3b0df4e50e1db56d8fbca3216eff4fb49 Mon Sep 17 00:00:00 2001 From: Alex Room Date: Fri, 11 Sep 2026 13:37:34 +0100 Subject: [PATCH 1/6] added string sample logs --- MNeuEventLib/plotting.py | 18 +- src/batch/functions.rs | 6 +- src/batch/interface.rs | 144 ++++++++++- src/data/nexus_data.rs | 73 ++++++ src/data/sample_logs.rs | 475 +++++++++++++++++++++++++++++------ src/data/save/sample_logs.rs | 5 +- src/filters/api.rs | 348 ++++++++++++++++++------- src/filters/mod.rs | 2 +- src/interface.rs | 20 ++ src/stats.rs | 2 +- src/test_utils.rs | 22 ++ src/utils.rs | 2 +- 12 files changed, 939 insertions(+), 178 deletions(-) diff --git a/MNeuEventLib/plotting.py b/MNeuEventLib/plotting.py index 9063779b..5d99834e 100644 --- a/MNeuEventLib/plotting.py +++ b/MNeuEventLib/plotting.py @@ -21,12 +21,20 @@ def plot_sample_log(data: Data, log_name: str): fig, ax = plt.subplots() ax.set_title(sample_log['name']) ax.set_xlabel("Time (seconds)") - if units := sample_log['unit'] == "": - unit_string = "" - else: - unit_string = f"({units})" + units = sample_log['unit'] + unit_string = "" if units == "" else f"({units})" ax.set_ylabel(f"Value {unit_string}") - ax.plot(sample_log['time'], sample_log['value']) + + values = sample_log['value'] + if isinstance(values, list): + # a string log comes back as a list of str rather than an array, so + # we need to plot it differently + categories = list(dict.fromkeys(values)) + ax.plot(sample_log['time'], values, drawstyle="steps-post", marker="o") + ax.set_yticks(categories) + ax.set_ylabel("Value") + else: + ax.plot(sample_log['time'], values) plt.show() diff --git a/src/batch/functions.rs b/src/batch/functions.rs index 7b94392e..5d9fa126 100644 --- a/src/batch/functions.rs +++ b/src/batch/functions.rs @@ -151,6 +151,7 @@ pub fn slog_range(name: String, log: String, start: f64, end: f64, step: f64) -> mod tests { use super::*; use crate::consts::ToNanoseconds; + use crate::filters::LogPredicate; /// The bounds of every filter set's single time filter, in order of /// filter set. @@ -173,7 +174,10 @@ mod tests { .map(|filters| { assert_eq!(filters.sample_log_filters.len(), 1); let filter = &filters.sample_log_filters[name]; - (filter.lower.unwrap(), filter.upper.unwrap()) + let LogPredicate::Range { lower, upper } = &filter.predicate else { + panic!("expected a range log filter") + }; + (lower.unwrap(), upper.unwrap()) }) .collect() } diff --git a/src/batch/interface.rs b/src/batch/interface.rs index 96e2c2d7..0cb1085a 100644 --- a/src/batch/interface.rs +++ b/src/batch/interface.rs @@ -1,8 +1,8 @@ use crate::data::save::sanitise::nexus_data::{ get_default_ref_file_path, get_period_info, save_default, }; -use crate::data::{NexusData, SaveFile, WiMDAFile}; -use crate::filters::Filters; +use crate::data::{NexusData, SampleLog, SaveFile, WiMDAFile}; +use crate::filters::{Filters, LogPredicate}; use crate::stats::Histogram; use anyhow::{Error, Result}; use ndarray::Array1; @@ -310,6 +310,13 @@ impl BatchData { lower: f64, upper: f64, ) -> Result<()> { + self.validate_log_filter( + &log, + &LogPredicate::Range { + lower: Some(lower), + upper: Some(upper), + }, + )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter(name.clone(), log.clone(), Some(lower), Some(upper))?; self.data_changed[i] = true; @@ -317,6 +324,33 @@ impl BatchData { Ok(()) } + /// Add a sample log filter matching a string log against a specific value. + /// + /// Parameters + /// ---------- + /// index: int | str + /// Either 'all', or the index of the filter set to modify. + /// name: str + /// The name of the log filter. Must be unique within each modified filter set. + /// log: str + /// The sample log in the data to which the filter applies. Must hold text. + /// value: str + /// The value to match. Matching is case-insensitive. + pub fn add_string_log_filter( + &mut self, + index: FilterIndex, + name: String, + log: String, + value: String, + ) -> Result<()> { + self.validate_log_filter(&log, &LogPredicate::Equals(value.clone()))?; + for i in self.resolve_indices(&index)? { + self.filters[i].add_string_log_filter(name.clone(), log.clone(), value.clone())?; + self.data_changed[i] = true; + } + Ok(()) + } + /// Edit an existing sample log filter. /// /// Parameters @@ -387,6 +421,13 @@ impl BatchData { log: String, lower: f64, ) -> Result<()> { + self.validate_log_filter( + &log, + &LogPredicate::Range { + lower: Some(lower), + upper: None, + }, + )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter_above(name.clone(), log.clone(), lower)?; self.data_changed[i] = true; @@ -413,6 +454,13 @@ impl BatchData { log: String, upper: f64, ) -> Result<()> { + self.validate_log_filter( + &log, + &LogPredicate::Range { + lower: None, + upper: Some(upper), + }, + )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter_below(name.clone(), log.clone(), upper)?; self.data_changed[i] = true; @@ -796,6 +844,30 @@ impl BatchData { } } + /// Check that a log filter can actually be applied to the data. + fn validate_log_filter(&self, log: &str, predicate: &LogPredicate) -> Result<()> { + // there is nothing to validate the filter against until a dataset is loaded + let Some(dataset) = &self.dataset else { + return Ok(()); + }; + let sample_log = dataset.get_sample_log(&log.to_string())?; + match (sample_log, predicate) { + (SampleLog::Str(_), LogPredicate::Equals(_)) => Ok(()), + (SampleLog::Str(_), LogPredicate::Range { .. }) => Err(Error::msg(format!( + "Sample log {} holds text, so it cannot be filtered on a range of \ + values. Use add_string_log_filter to filter it on a specific value.", + log, + ))), + (_, LogPredicate::Equals(_)) => Err(Error::msg(format!( + "Sample log {} holds numbers, so it cannot be filtered on a \ + string value. Use add_log_filter, add_log_filter_above or \ + add_log_filter_below.", + log, + ))), + _ => Ok(()), + } + } + /// Check that a given index is valid for this BatchData's filter sets. /// /// Parameters @@ -969,14 +1041,36 @@ impl BatchData { #[cfg(test)] mod tests { + use std::str::FromStr; + use super::*; use crate::test_utils::MockData; + use hdf5::types::VarLenUnicode; use ndarray::Array1; /// Build a BatchData with `n` filter sets using MockData as the /// underlying dataset (no real .nxs file needed). fn make_batch(n_filter_sets: usize) -> BatchData { let mock = MockData::new().unwrap(); + // log filters are validated against the data when they are added, so the mock + // needs to actually contain the logs these tests filter on + mock.add_sample_log( + "temp", + Array1::from_vec(vec![0., 1., 2., 3.]), + Array1::from_vec(vec![0.5, 1.5, 2.5, 3.5]), + ) + .unwrap(); + mock.add_sample_log( + "status", + Array1::from_vec(vec![0., 1., 2., 3.]), + Array1::from_vec( + ["IDLE", "RUNNING", "RUNNING", "PAUSED"] + .iter() + .map(|s| VarLenUnicode::from_str(s).unwrap()) + .collect::>(), + ), + ) + .unwrap(); let dataset = mock.create(64, 1048576).unwrap(); BatchData::from_dataset(dataset, n_filter_sets) } @@ -1147,8 +1241,13 @@ mod tests { filters.get_required_log_names(), ["temp".to_string()].into() ); - assert_eq!(filters.sample_log_filters["lf1"].lower, Some(array[k])); - assert_eq!(filters.sample_log_filters["lf1"].upper, Some(array[k + 1])); + assert_eq!( + filters.sample_log_filters["lf1"].predicate, + LogPredicate::Range { + lower: Some(array[k]), + upper: Some(array[k + 1]) + } + ); } } @@ -1416,6 +1515,43 @@ mod tests { } } + /// Adding a string log filter should apply it to the named filter sets. + #[test] + fn test_add_string_log_filter() { + let mut batch = make_batch(2); + batch + .add_string_log_filter( + FilterIndex::All, + "running".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + for filters in &batch.filters { + assert_eq!( + filters.get_required_log_names(), + ["status".to_string()].into() + ); + } + } + + /// A filter naming a log that isn't in the data is rejected when it is added, + /// rather than at calculate time. + #[test] + fn test_add_log_filter_missing_log() { + let mut batch = make_batch(1); + let result = batch.add_log_filter( + FilterIndex::Index(0), + "broken".to_string(), + "nonexistent".to_string(), + 1.0, + 2.0, + ); + + assert!(result.is_err()); + } + /// Setting an amplitude filter at a single index should only affect /// that filter set's amplitude array. #[test] diff --git a/src/data/nexus_data.rs b/src/data/nexus_data.rs index 0c05d7c1..15ead6e6 100644 --- a/src/data/nexus_data.rs +++ b/src/data/nexus_data.rs @@ -66,6 +66,19 @@ impl NexusData { output.set_item("unit".to_string(), log.unit)?; } )+ + // String logs return `value` as a list[str] rather than an ndarray + SampleLog::Str(log) => { + output.set_item("name".to_string(), log.name)?; + output.set_item("time".to_string(), log.time.to_pyarray(py))?; + output.set_item( + "value".to_string(), + log.value + .iter() + .map(|v| v.to_string()) + .collect::>(), + )?; + output.set_item("unit".to_string(), log.unit)?; + } } } } @@ -270,8 +283,11 @@ fn load_data(filename: &Path, n_spec: usize, chunk_size: usize) -> Result (NexusData, std::sync::MutexGuard<'static, ()>) { let guard = crate::test_utils::lock_hdf5_test(); @@ -326,6 +342,63 @@ mod tests { assert!(log.is_err()) } + /// Test a variable-length UTF-8 sample log is read. + #[test] + fn test_load_string_sample_log() { + let mock = MockData::new().unwrap(); + mock.add_sample_log( + "status", + Array1::from_vec(vec![0., 3., 10.]), + Array1::from_vec( + ["IDLE", "RUNNING", "PAUSED"] + .iter() + .map(|s| VarLenUnicode::from_str(s).unwrap()) + .collect::>(), + ), + ) + .unwrap(); + let data = mock.create(64, 1048576).unwrap(); + + let log = data.get_sample_log(&"status".to_string()).unwrap(); + + match log { + SampleLog::Str(log) => { + let values: Vec = log.value.iter().map(|v| v.to_string()).collect(); + assert_eq!(values, vec!["IDLE", "RUNNING", "PAUSED"]); + assert_eq!(log.time, Array1::from_vec(vec![0., 3., 10.])); + } + _ => panic!("a string sample log should load as SampleLog::Str"), + } + } + + /// Test an ASCII string log is normalised onto the same Str variant + #[test] + fn test_load_ascii_sample_log_normalises_to_unicode() { + let mock = MockData::new().unwrap(); + mock.add_sample_log( + "status", + Array1::from_vec(vec![0., 1.]), + Array1::from_vec( + ["IDLE", "RUNNING"] + .iter() + .map(|s| VarLenAscii::from_ascii(s).unwrap()) + .collect::>(), + ), + ) + .unwrap(); + let data = mock.create(64, 1048576).unwrap(); + + let log = data.get_sample_log(&"status".to_string()).unwrap(); + + match log { + SampleLog::Str(log) => { + let values: Vec = log.value.iter().map(|v| v.to_string()).collect(); + assert_eq!(values, vec!["IDLE", "RUNNING"]); + } + _ => panic!("an ASCII sample log should load as SampleLog::Str"), + } + } + /// Test that an amplitude histogram is successfully created for some data. /// This test uses a fixed maximum. #[test] diff --git a/src/data/sample_logs.rs b/src/data/sample_logs.rs index 6558426b..422aa12d 100644 --- a/src/data/sample_logs.rs +++ b/src/data/sample_logs.rs @@ -1,11 +1,12 @@ use std::str::FromStr; use anyhow::{Error, Result}; -use hdf5::types::{FloatSize, IntSize, TypeDescriptor, VarLenUnicode}; +use hdf5::types::{FloatSize, IntSize, TypeDescriptor, VarLenAscii, VarLenUnicode}; use hdf5::Dataset; use ndarray::Array1; use crate::consts::ToNanoseconds; +use crate::filters::LogPredicate; // Pattern-matching is the only way to access the internal value log, // so this macro lets you call a method of ValueLog from inside the @@ -23,6 +24,7 @@ macro_rules! bind { SampleLog::U64(log) => SampleLog::U64(log.$method($($arg),*)), SampleLog::F32(log) => SampleLog::F32(log.$method($($arg),*)), SampleLog::F64(log) => SampleLog::F64(log.$method($($arg),*)), + SampleLog::Str(log) => SampleLog::Str(log.$method($($arg),*)), } } } @@ -39,12 +41,59 @@ pub enum SampleLog { U64(ValueLog), F32(ValueLog), F64(ValueLog), + // string logs are always stored as VarLenUnicode + Str(ValueLog), +} + +/// Read the `units` attribute of a value dataset, defaulting to no units. +fn read_unit(value: &Dataset) -> String { + match value.attr("units") { + Ok(units) => units + .read_scalar::() + .map(|u| u.to_string()) + .unwrap_or_default(), + Err(_) => String::new(), + } } impl SampleLog { /// Create a new SampleLog. pub fn new(log_name: &String, time: Array1, value: Dataset) -> Result { let dtype = value.dtype()?.to_descriptor()?; + let unit = read_unit(&value); + + // String logs are handled before the numeric macro because both variable-length + // encodings normalise onto a single Str variant, so they do not fit the + // one-HDF5-type-per-Rust-type shape the macro assumes. + // + // Fixed-length strings are deliberately not supported: FixedAscii/FixedUnicode + // carry N as a const generic while TypeDescriptor::FixedAscii(usize) carries it as a + // runtime value, so dispatching between them needs a hardcoded ladder of sizes. + // ISIS writes variable-length UTF-8, so this buys nothing. + match dtype { + TypeDescriptor::VarLenUnicode => { + return Ok(SampleLog::Str(ValueLog:: { + name: log_name.clone(), + time, + value: value.read_1d()?, + unit, + })) + } + TypeDescriptor::VarLenAscii => { + let ascii: Array1 = value.read_1d()?; + let values = ascii + .iter() + .map(|s| VarLenUnicode::from_str(s)) + .collect::, _>>()?; + return Ok(SampleLog::Str(ValueLog:: { + name: log_name.clone(), + time, + value: values.into(), + unit, + })); + } + _ => {} + } // this macro just lets us write the type conversions, // and it handles the actual construction which is always the same @@ -60,21 +109,17 @@ impl SampleLog { match dtype { $( $hdf5_type => { - let unit: VarLenUnicode = match value.attr("units") { - Ok(units) => units.read().unwrap().into_scalar(), - Err(_) => VarLenUnicode::from_str("").unwrap(), - }; SampleLog::$variant(ValueLog::<$type> { name: log_name.clone(), time, value: value.read_1d()?, - unit: unit.to_string() + unit }) }, )+ other_type => return Err(Error::msg(format!( "Sample log type {other_type} for log {log_name} is not supported. - Supported types are Integer and Float.", + Supported types are Integer, Float and variable-length String.", ))) } }; @@ -94,28 +139,93 @@ impl SampleLog { Ok(log) } - /// Given a lower and upper limit, get the list of time starts and ends - /// corresponding to the log filter. - pub fn to_time_ranges(&self, lower: f64, upper: f64) -> (Vec, Vec) { + /// The name of the log. + pub fn name(&self) -> &str { match self { - SampleLog::I8(log) => log.to_time_ranges(&(lower as i8), &(upper as i8)), - SampleLog::I16(log) => log.to_time_ranges(&(lower as i16), &(upper as i16)), - SampleLog::I32(log) => log.to_time_ranges(&(lower as i32), &(upper as i32)), - SampleLog::I64(log) => log.to_time_ranges(&(lower as i64), &(upper as i64)), - SampleLog::U8(log) => log.to_time_ranges(&(lower as u8), &(upper as u8)), - SampleLog::U16(log) => log.to_time_ranges(&(lower as u16), &(upper as u16)), - SampleLog::U32(log) => log.to_time_ranges(&(lower as u32), &(upper as u32)), - SampleLog::U64(log) => log.to_time_ranges(&(lower as u64), &(upper as u64)), - SampleLog::F32(log) => log.to_time_ranges(&(lower as f32), &(upper as f32)), - SampleLog::F64(log) => log.to_time_ranges(&lower, &upper), + SampleLog::I8(log) => &log.name, + SampleLog::I16(log) => &log.name, + SampleLog::I32(log) => &log.name, + SampleLog::I64(log) => &log.name, + SampleLog::U8(log) => &log.name, + SampleLog::U16(log) => &log.name, + SampleLog::U32(log) => &log.name, + SampleLog::U64(log) => &log.name, + SampleLog::F32(log) => &log.name, + SampleLog::F64(log) => &log.name, + SampleLog::Str(log) => &log.name, } } - /// Given a list of filter starts and ends, return the sample log with filter applied. + /// The distinct values a string log takes, in order of first appearance. + pub fn distinct_values(&self) -> Option> { + match self { + SampleLog::Str(log) => { + let mut seen = Vec::::new(); + for value in log.value.iter() { + let value = value.to_string(); + if !seen.contains(&value) { + seen.push(value); + } + } + Some(seen) + } + _ => None, + } + } + + /// Given a filter predicate, get the list of time starts and ends + /// corresponding to the log filter. /// - /// Assumes that start_times and end_times are sorted arrays. - pub fn apply_filters(&self, start_times: &[u64], end_times: &[u64]) -> SampleLog { - bind! {self, apply_filters(start_times, end_times)} + /// Returns an error if the predicate does not suit the log's type, e.g. a range + /// filter applied to a log holding text + pub fn to_time_ranges(&self, predicate: &LogPredicate) -> Result<(Vec, Vec)> { + // this macro pairs every numeric variant with every predicate, so that the + // match stays exhaustive without a catch-all arm hiding a future variant + macro_rules! ranges_for_predicate { + ( $( $variant:ident : $type:ty ),+ $(,)? ) => { + match (self, predicate) { + (SampleLog::Str(log), LogPredicate::Equals(value)) => { + Ok(log.to_time_ranges_eq(value)) + } + (SampleLog::Str(log), LogPredicate::Range { .. }) => Err(Error::msg(format!( + "Sample log {} holds text, so it cannot be filtered on a range of \ + values. Use add_string_log_filter to filter it on a specific value.", + log.name, + ))), + $( + (SampleLog::$variant(log), LogPredicate::Range { lower, upper }) => { + Ok(log.to_time_ranges( + &(lower.unwrap_or(-f64::INFINITY) as $type), + &(upper.unwrap_or(f64::INFINITY) as $type), + )) + }, + (SampleLog::$variant(log), LogPredicate::Equals(_)) => { + Err(Error::msg(format!( + "Sample log {} holds numbers, so it cannot be filtered on a \ + string value. Use add_log_filter, add_log_filter_above or \ + add_log_filter_below.", + log.name, + ))) + }, + )+ + } + }; + } + ranges_for_predicate! { + I8: i8, I16: i16, I32: i32, I64: i64, + U8: u8, U16: u16, U32: u32, U64: u64, + F32: f32, F64: f64, + } + } + + /// Given a list of filter starts and ends, return the sample log with filter applied. + pub fn apply_filters( + &self, + start_times: &[u64], + end_times: &[u64], + include: bool, + ) -> SampleLog { + bind! {self, apply_filters(start_times, end_times, include)} } } @@ -156,64 +266,68 @@ where } } +impl ValueLog { + /// Internal implementation of SampleLog.to_time_ranges for a string log. + fn to_time_ranges_eq(&self, value: &str) -> (Vec, Vec) { + let mut starts = Vec::::new(); + let mut ends = Vec::::new(); + let mut in_range: bool = false; // whether we are currently in a matching run + // fold the needle once, rather than once per sample + let needle = value.to_lowercase(); + + for (index, entry) in self.value.iter().enumerate() { + let matches = entry.to_lowercase() == needle; + if !in_range { + if matches { + starts.push(self.time[index].to_ns()); + in_range = true; + } + } else if !matches { + // the run ends where the next value takes over, not at the previous sample + ends.push(self.time[index].to_ns()); + in_range = false; + } + } + + // if still matching at the end, the run ends at the last datapoint. Note this is + // the last logged time, not the end of the run - a log that stops being written + // early will truncate the filter here. + if in_range { + ends.push(self.time.last().unwrap().to_ns()); + } + (starts, ends) + } +} + impl ValueLog where T: Clone, { /// Internal implementation of SampleLog.apply_filters. - fn apply_filters(&self, start_times: &[u64], end_times: &[u64]) -> ValueLog { - // we use these indices to ignore overlaps in filters. - // - // Note that as the start_times and end_times are sorted, we will never get a scenario - // where one interval lies completely inside another: - // - // let (--) be one filter and [~~] another, - // then the filter pair [s1, e1], [s2, e2] like so - // s1 e1 - // (-------) - // [~~~] - // s2 e2 - // has starts [s1, s2], ends [e1, e2] - // which would be sorted as starts [s1, s2], ends [e2, e1] - // which parses as filters [s1, e2], [s2, e1] - // s1 e2 - // (-----) - // [~~~~~] - // s2 e1 - // - // This means we can ignore overlaps by checking if we've passed an extra start value, - // and if so, just skipping an end value to compensate - let mut current_start_idx = 0; - let mut current_end_idx = 0; - let mut in_filter = false; + fn apply_filters(&self, start_times: &[u64], end_times: &[u64], include: bool) -> ValueLog { + // Sort filters to make overlaps easier to handle + let mut ranges: Vec<(u64, u64)> = start_times + .iter() + .copied() + .zip(end_times.iter().copied()) + .collect(); + ranges.sort_unstable(); + let mut new_times = Vec::::with_capacity(self.time.len()); let mut new_values = Vec::::with_capacity(self.value.len()); - let max_start_idx = start_times.len() - 1; + let mut next_range = 0; + let mut reach: Option = None; for (k, t) in self.time.iter().enumerate() { let time_ns = t.to_ns(); - if in_filter { - if time_ns >= end_times[current_end_idx] { - // end of interval - in_filter = false; - current_end_idx += 1; - current_start_idx += 1; - new_times.push(*t); - new_values.push(self.value[k].clone()); - } else if current_start_idx < max_start_idx - && time_ns >= start_times[current_start_idx + 1] - { - // overlap detected - current_start_idx += 1; - current_end_idx += 1 - } - } else if current_start_idx < start_times.len() - && time_ns >= start_times[current_start_idx] - { - // start of interval - in_filter = true; - } else { - // not in filter; append to new log + while next_range < ranges.len() && ranges[next_range].0 <= time_ns { + let end = ranges[next_range].1; + reach = Some(reach.map_or(end, |r: u64| r.max(end))); + next_range += 1; + } + // ranges are closed at both ends, matching to_time_ranges + let in_range = reach.is_some_and(|end| time_ns <= end); + if in_range == include { new_times.push(*t); new_values.push(self.value[k].clone()); } @@ -231,6 +345,168 @@ where mod tests { use super::*; + /// Build a string value log from (time, value) pairs. + fn string_log(entries: &[(f64, &str)]) -> ValueLog { + ValueLog:: { + name: "status".to_string(), + time: entries.iter().map(|(t, _)| *t).collect::>().into(), + value: entries + .iter() + .map(|(_, v)| VarLenUnicode::from_str(v).unwrap()) + .collect::>() + .into(), + unit: "".to_string(), + } + } + + fn ns(seconds: f64) -> u64 { + seconds.to_ns() + } + + #[test] + fn test_str_time_ranges() { + let log = string_log(&[(0., "IDLE"), (3., "RUNNING"), (10., "PAUSED")]); + + let (starts, ends) = log.to_time_ranges_eq("RUNNING"); + + assert_eq!(starts, vec![ns(3.)]); + assert_eq!(ends, vec![ns(10.)]); + } + + /// A run still matching at the end of the log ends at the last logged time. + #[test] + fn test_str_time_ranges_at_end() { + let log = string_log(&[(0., "IDLE"), (3., "RUNNING"), (10., "RUNNING")]); + + let (starts, ends) = log.to_time_ranges_eq("RUNNING"); + + assert_eq!(starts, vec![ns(3.)]); + assert_eq!(ends, vec![ns(10.)]); + } + + /// A log whose first value already matches starts at its first timestamp. + #[test] + fn test_str_time_ranges_at_start() { + let log = string_log(&[(2., "RUNNING"), (5., "PAUSED")]); + + let (starts, ends) = log.to_time_ranges_eq("RUNNING"); + + assert_eq!(starts, vec![ns(2.)]); + assert_eq!(ends, vec![ns(5.)]); + } + + /// Alternating values produce one range per matching run. + #[test] + fn test_str_time_ranges_multiple_runs() { + let log = string_log(&[ + (0., "RUNNING"), + (1., "PAUSED"), + (2., "RUNNING"), + (3., "RUNNING"), + (4., "IDLE"), + ]); + + let (starts, ends) = log.to_time_ranges_eq("RUNNING"); + + assert_eq!(starts, vec![ns(0.), ns(2.)]); + assert_eq!(ends, vec![ns(1.), ns(4.)]); + } + + /// A value that never appears produces no ranges. + #[test] + fn test_str_time_ranges_no_match() { + let log = string_log(&[(0., "IDLE"), (3., "PAUSED")]); + + let (starts, ends) = log.to_time_ranges_eq("RUNNING"); + + assert!(starts.is_empty()); + assert!(ends.is_empty()); + } + + /// Matching ignores case, in both the log value and the requested value. + #[test] + fn test_time_ranges_eq_is_case_insensitive() { + let log = string_log(&[(0., "idle"), (3., "Running"), (10., "PAUSED")]); + + let (starts, ends) = log.to_time_ranges_eq("rUnNiNg"); + + assert_eq!(starts, vec![ns(3.)]); + assert_eq!(ends, vec![ns(10.)]); + } + + /// A string log cannot be filtered on a numeric range. + #[test] + fn test_range_predicate_on_string_log_errors() { + let log = SampleLog::Str(string_log(&[(0., "IDLE")])); + + let result = log.to_time_ranges(&LogPredicate::Range { + lower: Some(1.), + upper: Some(2.), + }); + + let error = result.unwrap_err().to_string(); + assert!(error.contains("holds text"), "unexpected error: {error}"); + assert!(error.contains("add_string_log_filter")); + } + + /// A numeric log cannot be filtered on a string value. + #[test] + fn test_equals_predicate_on_numeric_log_errors() { + let log = SampleLog::F64(ValueLog:: { + name: "temp".to_string(), + time: Array1::::linspace(0., 4., 5), + value: Array1::::linspace(0., 4., 5), + unit: "".to_string(), + }); + + let result = log.to_time_ranges(&LogPredicate::Equals("RUNNING".to_string())); + + let error = result.unwrap_err().to_string(); + assert!(error.contains("holds numbers"), "unexpected error: {error}"); + } + + /// distinct_values lists a string log's values in order of first appearance. + #[test] + fn test_distinct_values() { + let log = SampleLog::Str(string_log(&[ + (0., "IDLE"), + (1., "RUNNING"), + (2., "RUNNING"), + (3., "IDLE"), + (4., "PAUSED"), + ])); + + assert_eq!( + log.distinct_values(), + Some(vec![ + "IDLE".to_string(), + "RUNNING".to_string(), + "PAUSED".to_string() + ]) + ); + } + + /// Test apply_filters works on a string log. + #[test] + fn test_apply_filters_string_log() { + let log = SampleLog::Str(string_log(&[ + (0., "IDLE"), + (1., "RUNNING"), + (2., "PAUSED"), + (3., "IDLE"), + ])); + + let filtered = log.apply_filters(&[ns(1.)], &[ns(2.)], true); + + match filtered { + SampleLog::Str(log) => { + let values: Vec = log.value.iter().map(|v| v.to_string()).collect(); + assert_eq!(values, vec!["RUNNING".to_string(), "PAUSED".to_string()]); + } + _ => panic!("filtering a string log should return a string log"), + } + } + /// Test log_filter_times correctly gets the times from the log filters for a simple case. #[test] fn test_time_ranges_simple() { @@ -329,6 +605,22 @@ mod tests { } } + /// Test an include filter keeps exactly the values inside its ranges. + #[test] + fn test_apply_filters_include() { + let log = sample_log(); + + // filters are [0.15, 0.22], [0.55, 0.83] + // 0 1 2 3 4 5 6 7 8 9 values + // ^--^ ^--------^ include + let filter_starts = vec![0.15.to_ns(), 0.55.to_ns()]; + let filter_ends = vec![0.22.to_ns(), 0.83.to_ns()]; + let new_log = log.apply_filters(&filter_starts, &filter_ends, true); + + let expected_vals = Array1::::from_vec(vec![2., 6., 7., 8.]); + assert_eq!(new_log.value, expected_vals) + } + /// Test applying filters successfully 'flattens' filtered-out values. #[test] fn test_apply_filters() { @@ -339,7 +631,7 @@ mod tests { // ^--^ ^--------^ exclude let filter_starts = vec![0.15.to_ns(), 0.55.to_ns()]; let filter_ends = vec![0.22.to_ns(), 0.83.to_ns()]; - let new_log = log.apply_filters(&filter_starts, &filter_ends); + let new_log = log.apply_filters(&filter_starts, &filter_ends, false); let expected_vals = Array1::::from_vec(vec![0., 1., 3., 4., 5., 9.]); assert_eq!(new_log.value, expected_vals) @@ -356,12 +648,43 @@ mod tests { // ^--------^ let filter_starts = vec![0.06.to_ns(), 0.22.to_ns(), 0.71.to_ns()]; let filter_ends = vec![0.35.to_ns(), 0.56.to_ns(), 0.89.to_ns()]; - let new_log = log.apply_filters(&filter_starts, &filter_ends); + let new_log = log.apply_filters(&filter_starts, &filter_ends, false); let expected_vals = Array1::::from_vec(vec![0., 6., 7., 9.]); assert_eq!(new_log.value, expected_vals) } + /// An include filter over overlapping ranges keeps the union of them. + #[test] + fn test_apply_filters_overlap_include() { + let log = sample_log(); + + // filters are [0.06, 0.35], [0.22, 0.56], [0.71, 0.89] + // 0 1 2 3 4 5 6 7 8 9 values + // ^-------^ ^---^ include + // ^--------^ + let filter_starts = vec![0.06.to_ns(), 0.22.to_ns(), 0.71.to_ns()]; + let filter_ends = vec![0.35.to_ns(), 0.56.to_ns(), 0.89.to_ns()]; + let new_log = log.apply_filters(&filter_starts, &filter_ends, true); + + let expected_vals = Array1::::from_vec(vec![1., 2., 3., 4., 5., 8.]); + assert_eq!(new_log.value, expected_vals) + } + + /// Ranges arrive concatenated per filter, so they are not sorted; the result must + /// not depend on the order they are given in. + #[test] + fn test_apply_filters_unsorted_ranges() { + let log = sample_log(); + + let filter_starts = vec![0.55.to_ns(), 0.15.to_ns()]; + let filter_ends = vec![0.83.to_ns(), 0.22.to_ns()]; + let new_log = log.apply_filters(&filter_starts, &filter_ends, true); + + let expected_vals = Array1::::from_vec(vec![2., 6., 7., 8.]); + assert_eq!(new_log.value, expected_vals) + } + /// Test applying filters works when the first value is included in a filter. #[test] fn test_apply_filters_including_start() { @@ -372,7 +695,7 @@ mod tests { // ^----------------^ exclude let filter_starts = vec![0]; let filter_ends = vec![0.61.to_ns()]; - let new_log = log.apply_filters(&filter_starts, &filter_ends); + let new_log = log.apply_filters(&filter_starts, &filter_ends, false); let expected_vals = Array1::::from_vec(vec![7., 8., 9.]); assert_eq!(new_log.value, expected_vals) @@ -387,7 +710,7 @@ mod tests { // ^------------... exclude let filter_starts = vec![0.45.to_ns()]; let filter_ends = vec![u64::MAX]; - let new_log = log.apply_filters(&filter_starts, &filter_ends); + let new_log = log.apply_filters(&filter_starts, &filter_ends, false); let expected_vals = Array1::::from_vec(vec![0., 1., 2., 3., 4.]); assert_eq!(new_log.value, expected_vals) diff --git a/src/data/save/sample_logs.rs b/src/data/save/sample_logs.rs index a7ac60b2..a5d895e0 100644 --- a/src/data/save/sample_logs.rs +++ b/src/data/save/sample_logs.rs @@ -25,6 +25,7 @@ impl Save for SampleLog { SampleLog::U64(log) => log.save_with_narrowing(group), SampleLog::F32(log) => log.save_to_group(group), SampleLog::F64(log) => log.save_with_narrowing(group), + SampleLog::Str(log) => log.save_to_group(group), } } } @@ -83,7 +84,9 @@ pub fn get_all_sample_logs(event_data: &NexusData, filters: &Filters) -> Result< match event_data.get_sample_log(name) { Ok(sample_log) => match filter_starts.is_empty() { true => Some(sample_log), - false => Some(sample_log.apply_filters(&filter_starts, &filter_ends)), + // get_filter_times has already folded exclude filters into + // the include ranges it returns + false => Some(sample_log.apply_filters(&filter_starts, &filter_ends, true)), } Err(error) => { // if sample log is unsupported, ignore in output diff --git a/src/filters/api.rs b/src/filters/api.rs index f2f19834..71cc1999 100644 --- a/src/filters/api.rs +++ b/src/filters/api.rs @@ -30,15 +30,42 @@ pub struct Filter { end: f64, } -// LogFilter bounds are Options because serialisation doesn't support infinity or -infinity; -// we use None to represent those +/// The condition a sample log filter tests. +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub enum LogPredicate { + // Range bounds are Options because serialisation doesn't support infinity or + // -infinity; we use None to represent those + Range { + lower: Option, + upper: Option, + }, + /// Matches case-insensitively; only valid for logs holding text. + Equals(String), +} + +impl LogPredicate { + /// A human-readable description of the condition, for the filter table. + fn describe(&self) -> String { + match self { + LogPredicate::Range { lower, upper } => format!( + "{} to {}", + lower.unwrap_or(-f64::INFINITY), + upper.unwrap_or(f64::INFINITY) + ), + LogPredicate::Equals(value) => format!("== {value:?}"), + } + } +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct LogFilter { log: String, - pub lower: Option, - pub upper: Option, + pub predicate: LogPredicate, } +/// The start and end times matched by the filters on each log, keyed by log name. +pub type LogFilterTimes = HashMap, Vec)>; + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct Filters { time_filter_type: FilterType, @@ -94,29 +121,23 @@ impl Filters { } /// Get the start and end points of each log filter, for each log - pub fn get_log_filter_times( - &self, - logs: HashMap, - ) -> HashMap, Vec)> { - let mut results = HashMap::new(); - for (log_name, log) in logs.iter() { - // the zip/unzip is to convert it from - // Vec<(u64, u64)> to (Vec, Vec) - let (starts, ends) = self - .sample_log_filters - .values() - .filter(|f| f.log == *log_name) - .flat_map(|f| { - let (log_starts, log_ends) = log.to_time_ranges( - f.lower.unwrap_or(-f64::INFINITY), - f.upper.unwrap_or(f64::INFINITY), - ); - log_starts.into_iter().zip(log_ends) - }) - .unzip(); - results.insert(log_name.to_string(), (starts, ends)); + pub fn get_log_filter_times(&self, logs: HashMap) -> Result { + let mut results = LogFilterTimes::new(); + for (name, filter) in &self.sample_log_filters { + let log = logs.get(&filter.log).ok_or_else(|| { + Error::msg(format!( + "Filter {} refers to sample log {}, which does not exist.", + name, filter.log, + )) + })?; + let (s, e) = log.to_time_ranges(&filter.predicate)?; + // filters on the same log are unioned here; different logs are + // intersected later, in get_log_weights + let (starts, ends) = results.entry(filter.log.clone()).or_default(); + starts.extend(s); + ends.extend(e); } - results + Ok(results) } // Get the relevant log for each log filter. @@ -267,6 +288,25 @@ impl Filters { ) -> Result<()> { validate_log_bounds(lower, upper)?; + self.add_log_predicate(name, log, LogPredicate::Range { lower, upper }) + } + + /// Add a log filter matching a string log against a specific value. + pub fn add_string_log_filter( + &mut self, + name: String, + log: String, + value: String, + ) -> Result<()> { + self.add_log_predicate(name, log, LogPredicate::Equals(value)) + } + + fn add_log_predicate( + &mut self, + name: String, + log: String, + predicate: LogPredicate, + ) -> Result<()> { // check name isn't already in use if self .sample_log_filters @@ -277,7 +317,7 @@ impl Filters { } self.sample_log_filters - .insert(name, LogFilter { log, lower, upper }); + .insert(name, LogFilter { log, predicate }); Ok(()) } @@ -298,11 +338,28 @@ impl Filters { ))?; let log = log.unwrap_or(filter.log.clone()); - let lower = lower.or(filter.lower); - let upper = upper.or(filter.upper); - validate_log_bounds(lower, upper)?; + let predicate = match &filter.predicate { + LogPredicate::Range { + lower: old_lower, + upper: old_upper, + } => { + let lower = lower.or(*old_lower); + let upper = upper.or(*old_upper); + validate_log_bounds(lower, upper)?; + LogPredicate::Range { lower, upper } + } + LogPredicate::Equals(value) => { + if lower.is_some() || upper.is_some() { + return Err(Error::msg(format!( + "Log filter {} matches a string value, so it has no bounds to edit.", + name, + ))); + } + LogPredicate::Equals(value.clone()) + } + }; - *filter = LogFilter { log, lower, upper }; + *filter = LogFilter { log, predicate }; Ok(()) } @@ -378,14 +435,9 @@ impl Filters { "".to_string() } else { let mut log_builder = Builder::new(); - log_builder.push_record(["name", "log", "min", "max"]); + log_builder.push_record(["name", "log", "condition"]); for (name, filter) in &self.sample_log_filters { - log_builder.push_record([ - name, - &filter.log, - &filter.lower.unwrap_or(-f64::INFINITY).to_string(), - &filter.upper.unwrap_or(f64::INFINITY).to_string(), - ]); + log_builder.push_record([name, &filter.log, &filter.predicate.describe()]); } let log_table = log_builder.build(); format!("Sample log filters:\n{log_table}\n\n") @@ -491,24 +543,30 @@ mod tests { "a".to_string(), LogFilter { log: "temp".to_string(), - lower: Some(1.), - upper: Some(2.), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(2.), + }, }, ); sample_log_filters.insert( "b".to_string(), LogFilter { log: "pulse width".to_string(), - lower: Some(3.), - upper: Some(4.), + predicate: LogPredicate::Range { + lower: Some(3.), + upper: Some(4.), + }, }, ); sample_log_filters.insert( "c".to_string(), LogFilter { log: "pressure".to_string(), - lower: Some(5.), - upper: Some(6.), + predicate: LogPredicate::Range { + lower: Some(5.), + upper: Some(6.), + }, }, ); @@ -541,24 +599,30 @@ mod tests { "a".to_string(), LogFilter { log: "simple".to_string(), - lower: Some(2.), - upper: Some(3.), + predicate: LogPredicate::Range { + lower: Some(2.), + upper: Some(3.), + }, }, ); sample_log_filters.insert( "b".to_string(), LogFilter { log: "simple".to_string(), - lower: Some(0.), - upper: Some(1.), + predicate: LogPredicate::Range { + lower: Some(0.), + upper: Some(1.), + }, }, ); sample_log_filters.insert( "c".to_string(), LogFilter { log: "complex".to_string(), - lower: Some(2.), - upper: Some(8.), + predicate: LogPredicate::Range { + lower: Some(2.), + upper: Some(8.), + }, }, ); @@ -592,7 +656,7 @@ mod tests { logs.insert("simple".to_string(), SampleLog::F64(simple_log)); logs.insert("complex".to_string(), SampleLog::F64(complex_log)); - let log_times = filters.get_log_filter_times(logs); + let log_times = filters.get_log_filter_times(logs).unwrap(); let (simple_starts, simple_ends) = log_times["simple"].clone(); let simple_ranges: HashSet<(u64, u64)> = @@ -621,16 +685,20 @@ mod tests { "a".to_string(), LogFilter { log: "simple".to_string(), - lower: Some(2.), - upper: None, + predicate: LogPredicate::Range { + lower: Some(2.), + upper: None, + }, }, ); sample_log_filters.insert( "b".to_string(), LogFilter { log: "simple".to_string(), - lower: None, - upper: Some(3.), + predicate: LogPredicate::Range { + lower: None, + upper: Some(3.), + }, }, ); @@ -654,7 +722,7 @@ mod tests { let mut logs = HashMap::::new(); logs.insert("simple".to_string(), SampleLog::F64(simple_log)); - let (starts, ends) = filters.get_log_filter_times(logs)["simple"].clone(); + let (starts, ends) = filters.get_log_filter_times(logs).unwrap()["simple"].clone(); let ranges: HashSet<(u64, u64)> = starts.into_iter().zip(ends).collect(); let expected_ranges: HashSet<(u64, u64)> = @@ -974,14 +1042,88 @@ mod tests { "name".to_string(), LogFilter { log: "temp".to_string(), - lower: Some(0.), - upper: Some(1.), + predicate: LogPredicate::Range { + lower: Some(0.), + upper: Some(1.), + }, + }, + ); + + assert_eq!(filters.sample_log_filters, expected) + } + + /// Test adding a string log filter is stored correctly. + #[test] + fn test_add_string_log_filter() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "name".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + let mut expected = HashMap::::new(); + expected.insert( + "name".to_string(), + LogFilter { + log: "status".to_string(), + predicate: LogPredicate::Equals("RUNNING".to_string()), }, ); assert_eq!(filters.sample_log_filters, expected) } + /// Test `__repr__` shows the value a string log filter matches on. + #[test] + fn test_repr_string_log_filter() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "logfilter".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + let repr = filters.__repr__(); + assert!(repr.contains("logfilter")); + assert!(repr.contains("status")); + assert!(repr.contains("RUNNING")); + } + + /// Test a filter naming a log that is not in the data gives an error. + #[test] + fn test_log_filter_to_times_missing_log() { + let mut sample_log_filters = HashMap::::new(); + sample_log_filters.insert( + "a".to_string(), + LogFilter { + log: "nonexistent".to_string(), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(2.), + }, + }, + ); + + let filters = Filters { + time_filter_type: FilterType::Include, + time_filters: HashMap::::new(), + sample_log_filters, + amplitudes: HashMap::new(), + overwrite_type: OverwriteType::Free, + }; + + let error = filters + .get_log_filter_times(HashMap::::new()) + .unwrap_err() + .to_string(); + assert!(error.contains("nonexistent"), "{error}"); + } + /// Test adding multiple log filters. #[test] fn test_add_multiple_log_filter() { @@ -1001,24 +1143,30 @@ mod tests { "name".to_string(), LogFilter { log: "temp".to_string(), - lower: Some(0.), - upper: Some(1.), + predicate: LogPredicate::Range { + lower: Some(0.), + upper: Some(1.), + }, }, ); expected.insert( "name2".to_string(), LogFilter { log: "p".to_string(), - lower: Some(5.), - upper: None, + predicate: LogPredicate::Range { + lower: Some(5.), + upper: None, + }, }, ); expected.insert( "name3".to_string(), LogFilter { log: "temp".to_string(), - lower: None, - upper: Some(8.), + predicate: LogPredicate::Range { + lower: None, + upper: Some(8.), + }, }, ); @@ -1057,8 +1205,10 @@ mod tests { "filter1".to_string(), LogFilter { log: "temp".to_string(), - lower: Some(3.), - upper: Some(4.), + predicate: LogPredicate::Range { + lower: Some(3.), + upper: Some(4.), + }, }, ); @@ -1112,8 +1262,10 @@ mod tests { "filter2".to_string(), LogFilter { log: "pw".to_string(), - lower: Some(3.), - upper: Some(4.), + predicate: LogPredicate::Range { + lower: Some(3.), + upper: Some(4.), + }, }, ); @@ -1163,16 +1315,20 @@ mod tests { "log1".to_string(), LogFilter { log: "temp".to_string(), - lower: Some(0.), - upper: Some(1.) + predicate: LogPredicate::Range { + lower: Some(0.), + upper: Some(1.) + } } ), ( "log2".to_string(), LogFilter { log: "pw".to_string(), - lower: Some(2.), - upper: Some(3.) + predicate: LogPredicate::Range { + lower: Some(2.), + upper: Some(3.) + } } ), ]) @@ -1211,8 +1367,10 @@ mod tests { "log1".to_string(), LogFilter { log: "pw".to_string(), - lower: Some(2.), - upper: Some(3.) + predicate: LogPredicate::Range { + lower: Some(2.), + upper: Some(3.) + } } )]) ); @@ -1296,8 +1454,10 @@ mod tests { "filter1".to_string(), LogFilter { log: "pw".to_string(), - lower: Some(3.), - upper: Some(4.), + predicate: LogPredicate::Range { + lower: Some(3.), + upper: Some(4.), + }, }, ); @@ -1324,8 +1484,10 @@ mod tests { filters.sample_log_filters["filter1"], LogFilter { log: "temp".to_string(), - lower: Some(1.), - upper: Some(5.), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(5.), + }, } ); @@ -1336,8 +1498,10 @@ mod tests { filters.sample_log_filters["filter1"], LogFilter { log: "pw".to_string(), - lower: Some(1.), - upper: Some(5.), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(5.), + }, } ); } @@ -1358,8 +1522,10 @@ mod tests { filters.sample_log_filters["filter1"], LogFilter { log: "temp".to_string(), - lower: Some(2.), - upper: None, + predicate: LogPredicate::Range { + lower: Some(2.), + upper: None, + }, } ); } @@ -1393,8 +1559,10 @@ mod tests { filters.sample_log_filters["filter1"], LogFilter { log: "temp".to_string(), - lower: Some(1.), - upper: Some(2.), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(2.), + }, } ); } @@ -1492,8 +1660,10 @@ mod tests { "c".to_string(), LogFilter { log: "Temp".to_string(), - lower: Some(5.), - upper: Some(6.), + predicate: LogPredicate::Range { + lower: Some(5.), + upper: Some(6.), + }, }, ); @@ -1531,8 +1701,10 @@ mod tests { "c".to_string(), LogFilter { log: "Temp".to_string(), - lower: Some(5.), - upper: None, + predicate: LogPredicate::Range { + lower: Some(5.), + upper: None, + }, }, ); diff --git a/src/filters/mod.rs b/src/filters/mod.rs index 8d4db710..e2520e91 100644 --- a/src/filters/mod.rs +++ b/src/filters/mod.rs @@ -4,4 +4,4 @@ pub use weights::Weights; mod filtering; pub use filtering::{get_log_weights, get_weights}; mod api; -pub use api::Filters; +pub use api::{Filters, LogPredicate}; diff --git a/src/interface.rs b/src/interface.rs index 89142c0d..65d05f3e 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -168,6 +168,26 @@ impl Data { .add_log_filter(FilterIndex::Index(0), name, log, lower, upper) } + /// Add a sample log filter matching a string log against a specific value. + /// + /// Parameters + /// ---------- + /// name: str + /// The name of the log filter. Must be unique. + /// log: str + /// The sample log in the data to which the filter applies. Must hold text. + /// value: str + /// The value to match. Matching is case-insensitive. + pub fn add_string_log_filter( + &mut self, + name: String, + log: String, + value: String, + ) -> Result<()> { + self.inner + .add_string_log_filter(FilterIndex::Index(0), name, log, value) + } + /// Edit an existing sample log filter. /// /// Parameters diff --git a/src/stats.rs b/src/stats.rs index 8d68776b..266d84a2 100644 --- a/src/stats.rs +++ b/src/stats.rs @@ -99,7 +99,7 @@ impl Histogram { Ok(logs) => logs, Err(info) => return Err(Error::msg(format!("Failed to get logs: {info}"))), }; - let log_filter_times = filters.get_log_filter_times(value_logs); + let log_filter_times = filters.get_log_filter_times(value_logs)?; get_log_weights(log_filter_times, &frame_start_times) }; time_weights & log_weights diff --git a/src/test_utils.rs b/src/test_utils.rs index ce985782..7a60b993 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -59,6 +59,28 @@ impl MockData { Ok(dataset.create(name)?) } + /// Add a sample log to the mock data object. + /// + /// Builds the `/value_log/{time,value}` structure that NexusData expects, + /// in a real (temporary) HDF5 file - so tests using this exercise the actual + /// read path rather than constructing a SampleLog directly. + pub fn add_sample_log(&self, name: &str, time: Array1, value: Array1) -> Result<()> + where + T: H5Type, + { + let log_group = self.sample_logs.create_group(name)?; + let value_log = log_group.create_group("value_log")?; + value_log + .new_dataset_builder() + .with_data(&time) + .create("time")?; + value_log + .new_dataset_builder() + .with_data(&value) + .create("value")?; + Ok(()) + } + /// Turn the mock data object into a real NexusData object. pub fn create(&self, n_spec: usize, chunk_size: usize) -> Result { for field in FLOAT_EVENT_FIELDS { diff --git a/src/utils.rs b/src/utils.rs index 99249347..d6d7d624 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -64,7 +64,7 @@ pub fn get_filter_times(data: &NexusData, filters: &Filters) -> Result<(Vec Ok(logs) => logs, Err(info) => return Err(Error::msg(format!("Failed to get logs: {info}"))), }; - let log_times = filters.get_log_filter_times(value_logs); + let log_times = filters.get_log_filter_times(value_logs)?; let (log_starts, log_ends) = log_times .into_values() .reduce(|(acc_starts, acc_ends), (x_starts, x_ends)| { From 92ca7879280e865a5613ccfe75df59e74613f965 Mon Sep 17 00:00:00 2001 From: Alex Room Date: Fri, 11 Sep 2026 13:39:26 +0100 Subject: [PATCH 2/6] update tutorial --- docs/source/how-to/filtering.ipynb | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/source/how-to/filtering.ipynb b/docs/source/how-to/filtering.ipynb index 20ec5abd..25d93374 100644 --- a/docs/source/how-to/filtering.ipynb +++ b/docs/source/how-to/filtering.ipynb @@ -222,6 +222,38 @@ "data.remove_log_filter(\"temperature filter 2\")" ] }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "### Filtering text sample logs\n", + "\n", + "Not every sample log holds numbers. Some record the state of a piece of equipment as text, for example a cryostat writing `\"IDLE\"`, `\"RUNNING\"` and `\"PAUSED\"` as the run progresses. A range makes no sense for these, so they are filtered on an exact value instead, using `add_string_log_filter`. It takes three arguments:\n", + "- `name`: A name label for the filter, unique across all sample log filters.\n", + "- `log`: The sample log that the filter applies to.\n", + "- `value`: The value the log must hold for data to be kept.\n", + "\n", + "```python\n", + "data.add_string_log_filter(\"running only\", \"status\", \"RUNNING\")\n", + "```\n", + "\n", + "This keeps only the data taken while the `status` log held `\"RUNNING\"`, and discards everything else. Matching ignores case, so `\"running\"` and `\"Running\"` select the same data.\n", + "\n", + "So a log reading\n", + "\n", + "| time | value |\n", + "| ---- | ----- |\n", + "| 0s | IDLE |\n", + "| 3s | RUNNING |\n", + "| 10s | PAUSED |\n", + "\n", + "filtered on `\"RUNNING\"` keeps the data from 3s to 10s.\n", + "\n", + "Text and numeric filters cannot be swapped: `add_log_filter` on a text log, or `add_string_log_filter` on a numeric one, raises an error saying so. Asking for a value the log never takes is also an error, which lists the values that *are* present - otherwise the filter would match nothing and you would silently get the whole dataset back.\n", + "\n", + "Text log filters are removed with `remove_log_filter`, just like numeric ones.\n" + ] + }, { "cell_type": "markdown", "id": "1383b7e2-cd11-4a83-a86c-6c4f98a889b0", From c58ff909c90e01f22f6eb045c51fb4cb94d2263e Mon Sep 17 00:00:00 2001 From: Alex Room Date: Fri, 2 Oct 2026 13:44:13 +0100 Subject: [PATCH 3/6] remove validate as it doesn't work with changeable datasets --- src/batch/interface.rs | 51 +++--------------------------------------- 1 file changed, 3 insertions(+), 48 deletions(-) diff --git a/src/batch/interface.rs b/src/batch/interface.rs index 0cb1085a..8938298c 100644 --- a/src/batch/interface.rs +++ b/src/batch/interface.rs @@ -1,8 +1,8 @@ use crate::data::save::sanitise::nexus_data::{ get_default_ref_file_path, get_period_info, save_default, }; -use crate::data::{NexusData, SampleLog, SaveFile, WiMDAFile}; -use crate::filters::{Filters, LogPredicate}; +use crate::data::{NexusData, SaveFile, WiMDAFile}; +use crate::filters::Filters; use crate::stats::Histogram; use anyhow::{Error, Result}; use ndarray::Array1; @@ -310,13 +310,6 @@ impl BatchData { lower: f64, upper: f64, ) -> Result<()> { - self.validate_log_filter( - &log, - &LogPredicate::Range { - lower: Some(lower), - upper: Some(upper), - }, - )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter(name.clone(), log.clone(), Some(lower), Some(upper))?; self.data_changed[i] = true; @@ -343,7 +336,6 @@ impl BatchData { log: String, value: String, ) -> Result<()> { - self.validate_log_filter(&log, &LogPredicate::Equals(value.clone()))?; for i in self.resolve_indices(&index)? { self.filters[i].add_string_log_filter(name.clone(), log.clone(), value.clone())?; self.data_changed[i] = true; @@ -421,13 +413,6 @@ impl BatchData { log: String, lower: f64, ) -> Result<()> { - self.validate_log_filter( - &log, - &LogPredicate::Range { - lower: Some(lower), - upper: None, - }, - )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter_above(name.clone(), log.clone(), lower)?; self.data_changed[i] = true; @@ -454,13 +439,6 @@ impl BatchData { log: String, upper: f64, ) -> Result<()> { - self.validate_log_filter( - &log, - &LogPredicate::Range { - lower: None, - upper: Some(upper), - }, - )?; for i in self.resolve_indices(&index)? { self.filters[i].add_log_filter_below(name.clone(), log.clone(), upper)?; self.data_changed[i] = true; @@ -844,30 +822,6 @@ impl BatchData { } } - /// Check that a log filter can actually be applied to the data. - fn validate_log_filter(&self, log: &str, predicate: &LogPredicate) -> Result<()> { - // there is nothing to validate the filter against until a dataset is loaded - let Some(dataset) = &self.dataset else { - return Ok(()); - }; - let sample_log = dataset.get_sample_log(&log.to_string())?; - match (sample_log, predicate) { - (SampleLog::Str(_), LogPredicate::Equals(_)) => Ok(()), - (SampleLog::Str(_), LogPredicate::Range { .. }) => Err(Error::msg(format!( - "Sample log {} holds text, so it cannot be filtered on a range of \ - values. Use add_string_log_filter to filter it on a specific value.", - log, - ))), - (_, LogPredicate::Equals(_)) => Err(Error::msg(format!( - "Sample log {} holds numbers, so it cannot be filtered on a \ - string value. Use add_log_filter, add_log_filter_above or \ - add_log_filter_below.", - log, - ))), - _ => Ok(()), - } - } - /// Check that a given index is valid for this BatchData's filter sets. /// /// Parameters @@ -1044,6 +998,7 @@ mod tests { use std::str::FromStr; use super::*; + use crate::filters::LogPredicate; use crate::test_utils::MockData; use hdf5::types::VarLenUnicode; use ndarray::Array1; From b3851c31211698a481810549ea92b145deec3f22 Mon Sep 17 00:00:00 2001 From: Alex Room Date: Fri, 2 Oct 2026 13:52:52 +0100 Subject: [PATCH 4/6] fixed tests --- src/batch/functions.rs | 8 ++++---- src/batch/interface.rs | 19 +------------------ 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/src/batch/functions.rs b/src/batch/functions.rs index 5d9fa126..34d93cf5 100644 --- a/src/batch/functions.rs +++ b/src/batch/functions.rs @@ -174,10 +174,10 @@ mod tests { .map(|filters| { assert_eq!(filters.sample_log_filters.len(), 1); let filter = &filters.sample_log_filters[name]; - let LogPredicate::Range { lower, upper } = &filter.predicate else { - panic!("expected a range log filter") - }; - (lower.unwrap(), upper.unwrap()) + match &filter.predicate { + LogPredicate::Range { lower, upper } => (lower.unwrap(), upper.unwrap()), + other => panic!("expected a range predicate, got {other:?}"), + } }) .collect() } diff --git a/src/batch/interface.rs b/src/batch/interface.rs index 8938298c..1d1fca55 100644 --- a/src/batch/interface.rs +++ b/src/batch/interface.rs @@ -1007,8 +1007,7 @@ mod tests { /// underlying dataset (no real .nxs file needed). fn make_batch(n_filter_sets: usize) -> BatchData { let mock = MockData::new().unwrap(); - // log filters are validated against the data when they are added, so the mock - // needs to actually contain the logs these tests filter on + // the mock needs to actually contain the logs these tests filter on mock.add_sample_log( "temp", Array1::from_vec(vec![0., 1., 2., 3.]), @@ -1491,22 +1490,6 @@ mod tests { } } - /// A filter naming a log that isn't in the data is rejected when it is added, - /// rather than at calculate time. - #[test] - fn test_add_log_filter_missing_log() { - let mut batch = make_batch(1); - let result = batch.add_log_filter( - FilterIndex::Index(0), - "broken".to_string(), - "nonexistent".to_string(), - 1.0, - 2.0, - ); - - assert!(result.is_err()); - } - /// Setting an amplitude filter at a single index should only affect /// that filter set's amplitude array. #[test] From 3299822e81323e67b42d957a7068d8f94b9980a5 Mon Sep 17 00:00:00 2001 From: Alex Room Date: Thu, 15 Oct 2026 09:26:24 +0100 Subject: [PATCH 5/6] review fixes --- src/batch/interface.rs | 78 ++++++++++++++++++++++------------------- src/data/sample_logs.rs | 52 ++++++++++++++++----------- src/filters/api.rs | 55 +++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 57 deletions(-) diff --git a/src/batch/interface.rs b/src/batch/interface.rs index 1d1fca55..c13c8498 100644 --- a/src/batch/interface.rs +++ b/src/batch/interface.rs @@ -500,8 +500,8 @@ impl BatchData { let data_changed = vec![true; results.len()]; let mut filters = self.filters.clone(); - for (i, filter) in filters.iter_mut().enumerate() { - filter.extend(other.filters[i].clone())? + for (i, filter_set) in filters.iter_mut().enumerate() { + filter_set.extend(other.filters[i].clone())? } Ok(BatchData { @@ -700,8 +700,8 @@ impl BatchData { /// filename: str /// The filename for the filters. pub fn load_filters(&mut self, index: usize, filename: String) -> Result<()> { - let filters = Filters::load(filename)?; - self.filters[index] = filters; + let filter_set = Filters::load(filename)?; + self.filters[index] = filter_set; self.data_changed[index] = true; Ok(()) } @@ -773,10 +773,10 @@ impl BatchData { Some(data) => data.__repr__(), None => "No data set.".to_string(), }; - for (i, (filters, results)) in self.filters.iter().zip(self.results.iter()).enumerate() { + for (i, (filter_set, results)) in self.filters.iter().zip(self.results.iter()).enumerate() { string += &format!( "\n\nFilter set {i}:\n{}{}", - filters.__repr__(), + filter_set.__repr__(), results.__repr__() ); } @@ -1088,8 +1088,8 @@ mod tests { .add_time_filter(FilterIndex::All, "f1".to_string(), 1.0, 2.0) .unwrap(); - for filters in &batch.filters { - let (starts, ends) = filters.get_time_filter_times(); + for filter_set in &batch.filters { + let (starts, ends) = filter_set.get_time_filter_times(); assert_eq!(starts, vec![1e9 as u64]); assert_eq!(ends, vec![2e9 as u64]); } @@ -1106,8 +1106,8 @@ mod tests { let result = batch.add_time_filter(FilterIndex::Index(5), "f1".to_string(), 1.0, 2.0); assert!(result.is_err()); - for (index, filters) in batch.filters.into_iter().enumerate() { - let (starts, ends) = filters.get_time_filter_times(); + for (index, filter_set) in batch.filters.into_iter().enumerate() { + let (starts, ends) = filter_set.get_time_filter_times(); if index == 1 { // converted to ns assert_eq!(starts, vec![0.5e9 as u64]); @@ -1174,8 +1174,8 @@ mod tests { .array_to_time_filters("f1".to_string(), array.clone()) .unwrap(); - for (i, filters) in batch.filters.iter().enumerate() { - let (starts, ends) = filters.get_time_filter_times(); + for (i, filter_set) in batch.filters.iter().enumerate() { + let (starts, ends) = filter_set.get_time_filter_times(); assert_eq!(starts, vec![(array[i] * 1e9) as u64]); assert_eq!(ends, vec![(array[i + 1] * 1e9) as u64]); } @@ -1190,13 +1190,13 @@ mod tests { .array_to_log_filters("lf1".to_string(), "temp".to_string(), array.clone()) .unwrap(); - for (k, filters) in batch.filters.into_iter().enumerate() { + for (k, filter_set) in batch.filters.into_iter().enumerate() { assert_eq!( - filters.get_required_log_names(), + filter_set.get_required_log_names(), ["temp".to_string()].into() ); assert_eq!( - filters.sample_log_filters["lf1"].predicate, + filter_set.sample_log_filters["lf1"].predicate, LogPredicate::Range { lower: Some(array[k]), upper: Some(array[k + 1]) @@ -1330,9 +1330,9 @@ mod tests { ) .unwrap(); - for filters in &batch.filters { + for filter_set in &batch.filters { assert_eq!( - filters.get_required_log_names(), + filter_set.get_required_log_names(), ["temp".to_string()].into() ); } @@ -1464,8 +1464,8 @@ mod tests { .remove_log_filter(FilterIndex::All, "lf1".to_string()) .unwrap(); - for filters in &batch.filters { - assert!(filters.get_required_log_names().is_empty()); + for filter_set in &batch.filters { + assert!(filter_set.get_required_log_names().is_empty()); } } @@ -1482,9 +1482,9 @@ mod tests { ) .unwrap(); - for filters in &batch.filters { + for filter_set in &batch.filters { assert_eq!( - filters.get_required_log_names(), + filter_set.get_required_log_names(), ["status".to_string()].into() ); } @@ -1514,8 +1514,8 @@ mod tests { let mut batch = make_batch(3); batch.set_amp(FilterIndex::All, 2, 4.4).unwrap(); - for filters in &batch.filters { - let amps = filters.get_amps(6).unwrap(); + for filter_set in &batch.filters { + let amps = filter_set.get_amps(6).unwrap(); assert_eq!( amps, ndarray::Array1::from_vec(vec![0., 0., 4.4, 0., 0., 0.]) @@ -1540,8 +1540,8 @@ mod tests { assert_eq!(combined.__len__(), 5); assert_eq!(combined.results.len(), 5); - for (i, filters) in combined.filters.iter().enumerate() { - let (starts, ends) = filters.get_time_filter_times(); + for (i, filter_set) in combined.filters.iter().enumerate() { + let (starts, ends) = filter_set.get_time_filter_times(); if i < 2 { assert_eq!(starts, vec![1e9 as u64]); assert_eq!(ends, vec![2e9 as u64]); @@ -1587,13 +1587,16 @@ mod tests { assert_eq!(combined.results.len(), 3); assert_eq!(combined.data_changed, vec![true; 3]); - for (i, filters) in combined.filters.iter().enumerate() { + for (i, filter_set) in combined.filters.iter().enumerate() { // the time filter comes from this object's filter set i - let (starts, ends) = filters.get_time_filter_times(); + let (starts, ends) = filter_set.get_time_filter_times(); assert_eq!(starts, vec![(i as f64 * 1e9) as u64]); assert_eq!(ends, vec![((i + 1) as f64 * 1e9) as u64]); // the log filter comes from the other object's filter set i - assert_eq!(filters.get_required_log_names(), [format!("log{i}")].into()); + assert_eq!( + filter_set.get_required_log_names(), + [format!("log{i}")].into() + ); } } @@ -1677,13 +1680,16 @@ mod tests { for i in 0..2 { for j in 0..3 { let k = i * 3 + j; - let filters = &combined.filters[k]; + let filter_set = &combined.filters[k]; // the time filter comes from this object's filter set i - let (starts, ends) = filters.get_time_filter_times(); + let (starts, ends) = filter_set.get_time_filter_times(); assert_eq!(starts, vec![((2 * i + 1) as f64 * 1e9) as u64]); assert_eq!(ends, vec![((2 * i + 2) as f64 * 1e9) as u64]); // the log filter comes from the other object's filter set j - assert_eq!(filters.get_required_log_names(), [format!("log{j}")].into()); + assert_eq!( + filter_set.get_required_log_names(), + [format!("log{j}")].into() + ); } } } @@ -1710,11 +1716,11 @@ mod tests { let combined = batch.combinations(other).unwrap(); assert_eq!(combined.__len__(), 3); - for filters in &combined.filters { - let (starts, _) = filters.get_time_filter_times(); + for filter_set in &combined.filters { + let (starts, _) = filter_set.get_time_filter_times(); assert_eq!(starts, vec![1e9 as u64]); assert_eq!( - filters.get_required_log_names(), + filter_set.get_required_log_names(), ["temp".to_string()].into() ); } @@ -1825,8 +1831,8 @@ mod tests { .set_time_type(FilterIndex::All, "exclude".to_string()) .unwrap(); - for filters in &batch.filters { - assert!(!filters.is_include()); + for filter_set in &batch.filters { + assert!(!filter_set.is_include()); } } } diff --git a/src/data/sample_logs.rs b/src/data/sample_logs.rs index 422aa12d..17fbc29f 100644 --- a/src/data/sample_logs.rs +++ b/src/data/sample_logs.rs @@ -62,14 +62,12 @@ impl SampleLog { let dtype = value.dtype()?.to_descriptor()?; let unit = read_unit(&value); - // String logs are handled before the numeric macro because both variable-length - // encodings normalise onto a single Str variant, so they do not fit the - // one-HDF5-type-per-Rust-type shape the macro assumes. + // Handle string lengths separately to the macro as there are two string types // // Fixed-length strings are deliberately not supported: FixedAscii/FixedUnicode // carry N as a const generic while TypeDescriptor::FixedAscii(usize) carries it as a // runtime value, so dispatching between them needs a hardcoded ladder of sizes. - // ISIS writes variable-length UTF-8, so this buys nothing. + // ISIS writes variable-length UTF-8, so this costs nothing. match dtype { TypeDescriptor::VarLenUnicode => { return Ok(SampleLog::Str(ValueLog:: { @@ -80,8 +78,9 @@ impl SampleLog { })) } TypeDescriptor::VarLenAscii => { + // we read the ascii and then convert it to unicode to unify the string type let ascii: Array1 = value.read_1d()?; - let values = ascii + let values: Vec = ascii .iter() .map(|s| VarLenUnicode::from_str(s)) .collect::, _>>()?; @@ -305,7 +304,22 @@ where { /// Internal implementation of SampleLog.apply_filters. fn apply_filters(&self, start_times: &[u64], end_times: &[u64], include: bool) -> ValueLog { - // Sort filters to make overlaps easier to handle + // Sort filters to make overlaps easier to handle: if the start_times and end_times are sorted, + // we will never get a scenario where one interval lies completely inside another: + // + // let (--) be one filter and [~~] another, + // then the filter pair [s1, e1], [s2, e2] like so + // s1 e1 + // (-------) + // [~~~] + // s2 e2 + // has starts [s1, s2], ends [e1, e2] + // which would be sorted as starts [s1, s2], ends [e2, e1] + // which parses as filters [s1, e2], [s2, e1] + // s1 e2 + // (-----) + // [~~~~~] + // s2 e1 let mut ranges: Vec<(u64, u64)> = start_times .iter() .copied() @@ -359,18 +373,14 @@ mod tests { } } - fn ns(seconds: f64) -> u64 { - seconds.to_ns() - } - #[test] fn test_str_time_ranges() { let log = string_log(&[(0., "IDLE"), (3., "RUNNING"), (10., "PAUSED")]); let (starts, ends) = log.to_time_ranges_eq("RUNNING"); - assert_eq!(starts, vec![ns(3.)]); - assert_eq!(ends, vec![ns(10.)]); + assert_eq!(starts, vec![3.0.to_ns()]); + assert_eq!(ends, vec![10.0.to_ns()]); } /// A run still matching at the end of the log ends at the last logged time. @@ -380,8 +390,8 @@ mod tests { let (starts, ends) = log.to_time_ranges_eq("RUNNING"); - assert_eq!(starts, vec![ns(3.)]); - assert_eq!(ends, vec![ns(10.)]); + assert_eq!(starts, vec![3.0.to_ns()]); + assert_eq!(ends, vec![10.0.to_ns()]); } /// A log whose first value already matches starts at its first timestamp. @@ -391,8 +401,8 @@ mod tests { let (starts, ends) = log.to_time_ranges_eq("RUNNING"); - assert_eq!(starts, vec![ns(2.)]); - assert_eq!(ends, vec![ns(5.)]); + assert_eq!(starts, vec![2.0.to_ns()]); + assert_eq!(ends, vec![5.0.to_ns()]); } /// Alternating values produce one range per matching run. @@ -408,8 +418,8 @@ mod tests { let (starts, ends) = log.to_time_ranges_eq("RUNNING"); - assert_eq!(starts, vec![ns(0.), ns(2.)]); - assert_eq!(ends, vec![ns(1.), ns(4.)]); + assert_eq!(starts, vec![0.0.to_ns(), 2.0.to_ns()]); + assert_eq!(ends, vec![1.0.to_ns(), 4.0.to_ns()]); } /// A value that never appears produces no ranges. @@ -430,8 +440,8 @@ mod tests { let (starts, ends) = log.to_time_ranges_eq("rUnNiNg"); - assert_eq!(starts, vec![ns(3.)]); - assert_eq!(ends, vec![ns(10.)]); + assert_eq!(starts, vec![3.0.to_ns()]); + assert_eq!(ends, vec![10.0.to_ns()]); } /// A string log cannot be filtered on a numeric range. @@ -496,7 +506,7 @@ mod tests { (3., "IDLE"), ])); - let filtered = log.apply_filters(&[ns(1.)], &[ns(2.)], true); + let filtered = log.apply_filters(&[1.0.to_ns()], &[2.0.to_ns()], true); match filtered { SampleLog::Str(log) => { diff --git a/src/filters/api.rs b/src/filters/api.rs index 71cc1999..912c7dcb 100644 --- a/src/filters/api.rs +++ b/src/filters/api.rs @@ -492,7 +492,9 @@ fn validate_log_bounds(lower: Option, upper: Option) -> Result<()> { mod tests { use super::*; use std::env::temp_dir; + use std::str::FromStr; + use hdf5::types::VarLenUnicode; use ndarray::Array1; use crate::data::ValueLog; @@ -1124,6 +1126,59 @@ mod tests { assert!(error.contains("nonexistent"), "{error}"); } + /// Test a numeric log filter on a string log gives an error. + #[test] + fn test_numeric_log_filter_on_string_log_errors() { + let mut filters = Filters::new(); + filters + .add_log_filter("a".to_string(), "status".to_string(), Some(1.), Some(2.)) + .unwrap(); + + let mut logs = HashMap::::new(); + logs.insert( + "status".to_string(), + SampleLog::Str(ValueLog:: { + name: "status".to_string(), + time: Array1::::from_vec(vec![0., 1.]), + value: Array1::from_vec(vec![ + VarLenUnicode::from_str("IDLE").unwrap(), + VarLenUnicode::from_str("RUNNING").unwrap(), + ]), + unit: "".to_string(), + }), + ); + + let error = filters.get_log_filter_times(logs).unwrap_err().to_string(); + assert!(error.contains("holds text"), "{error}"); + } + + /// Test a string log filter on a numeric log gives an error. + #[test] + fn test_string_log_filter_on_numeric_log_errors() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "a".to_string(), + "temp".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + let mut logs = HashMap::::new(); + logs.insert( + "temp".to_string(), + SampleLog::F64(ValueLog:: { + name: "temp".to_string(), + time: Array1::::linspace(0., 4., 5), + value: Array1::::linspace(0., 4., 5), + unit: "".to_string(), + }), + ); + + let error = filters.get_log_filter_times(logs).unwrap_err().to_string(); + assert!(error.contains("holds numbers"), "{error}"); + } + /// Test adding multiple log filters. #[test] fn test_add_multiple_log_filter() { From 5a5271c0f37d397c042567b1d0a3fa17c7ca0a04 Mon Sep 17 00:00:00 2001 From: Alex Room Date: Thu, 15 Oct 2026 09:53:38 +0100 Subject: [PATCH 6/6] add edit string filter --- docs/source/how-to/filtering.ipynb | 2 + src/batch/interface.rs | 66 ++++++++++++ src/data/sample_logs.rs | 11 +- src/filters/api.rs | 159 +++++++++++++++++++++++++++-- src/interface.rs | 23 +++++ 5 files changed, 247 insertions(+), 14 deletions(-) diff --git a/docs/source/how-to/filtering.ipynb b/docs/source/how-to/filtering.ipynb index 25d93374..38875663 100644 --- a/docs/source/how-to/filtering.ipynb +++ b/docs/source/how-to/filtering.ipynb @@ -251,6 +251,8 @@ "\n", "Text and numeric filters cannot be swapped: `add_log_filter` on a text log, or `add_string_log_filter` on a numeric one, raises an error saying so. Asking for a value the log never takes is also an error, which lists the values that *are* present - otherwise the filter would match nothing and you would silently get the whole dataset back.\n", "\n", + "Text log filters are changed with `edit_string_log_filter`, which takes the name of the filter and new values for `log` and/or `value`; anything we don't give is left as it is. For example, `data.edit_string_log_filter(\"running only\", value=\"PAUSED\")` would make our filter keep the data from 10s onwards instead. `edit_log_filter` can't change the value of a text filter, and `edit_string_log_filter` can't edit a numeric one.\n", + "\n", "Text log filters are removed with `remove_log_filter`, just like numeric ones.\n" ] }, diff --git a/src/batch/interface.rs b/src/batch/interface.rs index c13c8498..764ee380 100644 --- a/src/batch/interface.rs +++ b/src/batch/interface.rs @@ -378,6 +378,37 @@ impl BatchData { Ok(()) } + /// Edit an existing string sample log filter. + /// + /// Parameters + /// ---------- + /// index: int | str + /// Either 'all', or the index of the filter set to modify. + /// name: str + /// The name of the string log filter to edit. Must already exist in + /// each modified filter set. + /// log: str | None + /// The new sample log for the filter to apply to. If None, the + /// sample log is left unchanged, otherwise the filter is moved to the + /// new sample log. Must hold text. + /// value: str | None + /// The new value to match. If None, the value is left unchanged. + /// Matching is case-insensitive. + #[pyo3(signature = (index, name, log=None, value=None))] + pub fn edit_string_log_filter( + &mut self, + index: FilterIndex, + name: String, + log: Option, + value: Option, + ) -> Result<()> { + for i in self.resolve_indices(&index)? { + self.filters[i].edit_string_log_filter(name.clone(), log.clone(), value.clone())?; + self.data_changed[i] = true; + } + Ok(()) + } + /// Remove a sample log filter. /// /// Parameters @@ -1490,6 +1521,41 @@ mod tests { } } + /// Editing a string log filter at a single index should only affect + /// that filter set. + #[test] + fn test_edit_string_log_filter_single_index() { + let mut batch = make_batch(2); + batch + .add_string_log_filter( + FilterIndex::All, + "running".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + batch.data_changed = vec![false; 2]; + batch + .edit_string_log_filter( + FilterIndex::Index(1), + "running".to_string(), + Some("state".to_string()), + None, + ) + .unwrap(); + + assert_eq!(batch.data_changed, vec![false, true]); + assert_eq!( + batch.filters[0].get_required_log_names(), + ["status".to_string()].into() + ); + assert_eq!( + batch.filters[1].get_required_log_names(), + ["state".to_string()].into() + ); + } + /// Setting an amplitude filter at a single index should only affect /// that filter set's amplitude array. #[test] diff --git a/src/data/sample_logs.rs b/src/data/sample_logs.rs index 17fbc29f..854f6220 100644 --- a/src/data/sample_logs.rs +++ b/src/data/sample_logs.rs @@ -62,7 +62,7 @@ impl SampleLog { let dtype = value.dtype()?.to_descriptor()?; let unit = read_unit(&value); - // Handle string lengths separately to the macro as there are two string types + // Handle string lengths separately to the macro as there are two string types // // Fixed-length strings are deliberately not supported: FixedAscii/FixedUnicode // carry N as a const generic while TypeDescriptor::FixedAscii(usize) carries it as a @@ -187,8 +187,7 @@ impl SampleLog { Ok(log.to_time_ranges_eq(value)) } (SampleLog::Str(log), LogPredicate::Range { .. }) => Err(Error::msg(format!( - "Sample log {} holds text, so it cannot be filtered on a range of \ - values. Use add_string_log_filter to filter it on a specific value.", + "Sample log {} holds text, so it cannot be filtered on a range of values. Use add_string_log_filter to filter it on a specific value.", log.name, ))), $( @@ -200,9 +199,7 @@ impl SampleLog { }, (SampleLog::$variant(log), LogPredicate::Equals(_)) => { Err(Error::msg(format!( - "Sample log {} holds numbers, so it cannot be filtered on a \ - string value. Use add_log_filter, add_log_filter_above or \ - add_log_filter_below.", + "Sample log {} holds numbers, so it cannot be filtered on a string value. Use add_log_filter, add_log_filter_above or add_log_filter_below.", log.name, ))) }, @@ -304,7 +301,7 @@ where { /// Internal implementation of SampleLog.apply_filters. fn apply_filters(&self, start_times: &[u64], end_times: &[u64], include: bool) -> ValueLog { - // Sort filters to make overlaps easier to handle: if the start_times and end_times are sorted, + // Sort filters to make overlaps easier to handle: if the start_times and end_times are sorted, // we will never get a scenario where one interval lies completely inside another: // // let (--) be one filter and [~~] another, diff --git a/src/filters/api.rs b/src/filters/api.rs index 912c7dcb..10b46a5a 100644 --- a/src/filters/api.rs +++ b/src/filters/api.rs @@ -351,8 +351,7 @@ impl Filters { LogPredicate::Equals(value) => { if lower.is_some() || upper.is_some() { return Err(Error::msg(format!( - "Log filter {} matches a string value, so it has no bounds to edit.", - name, + "Log filter {name} matches a string value, so it has no bounds to edit. Use edit_string_log_filter to change the value it matches." ))); } LogPredicate::Equals(value.clone()) @@ -363,6 +362,32 @@ impl Filters { Ok(()) } + /// Edit an existing string log filter. + /// + /// A field given as `None` is left as it is. + pub fn edit_string_log_filter( + &mut self, + name: String, + log: Option, + value: Option, + ) -> Result<()> { + let filter = self.sample_log_filters.get_mut(&name).ok_or(Error::msg( + "No such name in log filters. Use `print(filters)` to see a list of all filters.", + ))?; + + let LogPredicate::Equals(old_value) = &filter.predicate else { + return Err(Error::msg(format!( + "Log filter {name} matches a range of values, not a string value. Use edit_log_filter to change its bounds." + ))); + }; + + let log = log.unwrap_or(filter.log.clone()); + let predicate = LogPredicate::Equals(value.unwrap_or(old_value.clone())); + + *filter = LogFilter { log, predicate }; + Ok(()) + } + pub fn remove_log_filter(&mut self, name: String) -> Result<()> { match self.sample_log_filters.remove(&name) { Some(_) => Ok(()), @@ -1157,11 +1182,7 @@ mod tests { fn test_string_log_filter_on_numeric_log_errors() { let mut filters = Filters::new(); filters - .add_string_log_filter( - "a".to_string(), - "temp".to_string(), - "RUNNING".to_string(), - ) + .add_string_log_filter("a".to_string(), "temp".to_string(), "RUNNING".to_string()) .unwrap(); let mut logs = HashMap::::new(); @@ -1641,6 +1662,130 @@ mod tests { assert!(result.is_ok()); } + /// Test editing the bounds of a string log filter throws an error. + #[test] + fn test_edit_log_filter_bounds_on_string_filter() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "filter1".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + let result = filters.edit_log_filter("filter1".to_string(), None, Some(1.0), None); + assert!(result.is_err()); + } + + /// Test editing every field of a string log filter. + #[test] + fn test_edit_string_log_filter() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "filter1".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + filters + .edit_string_log_filter( + "filter1".to_string(), + Some("state".to_string()), + Some("PAUSED".to_string()), + ) + .unwrap(); + + assert_eq!( + filters.sample_log_filters["filter1"], + LogFilter { + log: "state".to_string(), + predicate: LogPredicate::Equals("PAUSED".to_string()), + } + ); + } + + /// Test fields of a string log filter given as None are left unchanged. + #[test] + fn test_edit_string_log_filter_partial() { + let mut filters = Filters::new(); + filters + .add_string_log_filter( + "filter1".to_string(), + "status".to_string(), + "RUNNING".to_string(), + ) + .unwrap(); + + filters + .edit_string_log_filter("filter1".to_string(), None, Some("PAUSED".to_string())) + .unwrap(); + assert_eq!( + filters.sample_log_filters["filter1"], + LogFilter { + log: "status".to_string(), + predicate: LogPredicate::Equals("PAUSED".to_string()), + } + ); + + filters + .edit_string_log_filter("filter1".to_string(), Some("state".to_string()), None) + .unwrap(); + assert_eq!( + filters.sample_log_filters["filter1"], + LogFilter { + log: "state".to_string(), + predicate: LogPredicate::Equals("PAUSED".to_string()), + } + ); + } + + /// Test editing a string log filter that doesn't exist throws an error. + #[test] + fn test_edit_string_log_filter_nonexistent() { + let mut filters = Filters::new(); + let result = filters.edit_string_log_filter( + "nonexistent".to_string(), + None, + Some("RUNNING".to_string()), + ); + assert!(result.is_err()); + } + + /// Test editing a range log filter as a string log filter throws an + /// error and leaves the filter as it was. + #[test] + fn test_edit_string_log_filter_on_range_filter() { + let mut filters = Filters::new(); + filters + .add_log_filter( + "filter1".to_string(), + "temp".to_string(), + Some(1.0), + Some(2.0), + ) + .unwrap(); + + let result = filters.edit_string_log_filter( + "filter1".to_string(), + Some("status".to_string()), + Some("RUNNING".to_string()), + ); + assert!(result.is_err()); + + assert_eq!( + filters.sample_log_filters["filter1"], + LogFilter { + log: "temp".to_string(), + predicate: LogPredicate::Range { + lower: Some(1.), + upper: Some(2.), + }, + } + ); + } + /// Test setting an amplitude for a given detector works. #[test] fn test_set_amp() { diff --git a/src/interface.rs b/src/interface.rs index 65d05f3e..ef619ea7 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -215,6 +215,29 @@ impl Data { .edit_log_filter(FilterIndex::Index(0), name, log, lower, upper) } + /// Edit an existing string sample log filter. + /// + /// Parameters + /// ---------- + /// name: str + /// The name of the string log filter to edit. Must already exist. + /// log: str | None + /// The new sample log for the filter to apply to. If None, the + /// sample log is left unchanged. Must hold text. + /// value: str | None + /// The new value to match. If None, the value is left unchanged. + /// Matching is case-insensitive. + #[pyo3(signature = (name, log=None, value=None))] + pub fn edit_string_log_filter( + &mut self, + name: String, + log: Option, + value: Option, + ) -> Result<()> { + self.inner + .edit_string_log_filter(FilterIndex::Index(0), name, log, value) + } + /// Remove a sample log filter. /// /// Parameters