Skip to content

Commit 4a60454

Browse files
authored
refactor: address nitpick comments in filters module (#35)
- Use BBLLog::duration_us() method instead of duplicate computation - Fix misleading comment on variance threshold logic - Add comprehensive unit tests for filtering heuristics (9 tests covering edge cases) - All tests, clippy, and formatting pass
1 parent fb3f39c commit 4a60454

1 file changed

Lines changed: 117 additions & 6 deletions

File tree

src/filters.rs

Lines changed: 117 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,8 @@ pub fn should_skip_export(log: &BBLLog, force_export: bool) -> (bool, String) {
3333
const FALLBACK_MIN_FRAMES: u32 = 7_500; // ~5 seconds at 1500 fps (fallback when no duration)
3434

3535
// Check if we have duration information
36-
if log.stats.start_time_us > 0 && log.stats.end_time_us > log.stats.start_time_us {
37-
let duration_us = log
38-
.stats
39-
.end_time_us
40-
.saturating_sub(log.stats.start_time_us);
36+
let duration_us = log.duration_us();
37+
if duration_us > 0 {
4138
// Use floating-point duration to avoid precision loss and division by zero
4239
let duration_s = duration_us as f64 / 1_000_000.0;
4340

@@ -168,7 +165,8 @@ pub fn has_minimal_gyro_activity(log: &BBLLog) -> (bool, f64) {
168165
// Use the maximum variance across all axes
169166
let max_variance = variance_x.max(variance_y).max(variance_z);
170167

171-
// Very conservative: only skip if ALL axes show extremely low variance
168+
// Very conservative: only skip if the highest variance across all axes is extremely low
169+
// This means the aircraft was essentially stationary (ground test)
172170
let is_minimal = max_variance < VERY_LOW_GYRO_VARIANCE_THRESHOLD;
173171

174172
(is_minimal, max_variance)
@@ -191,3 +189,116 @@ pub fn calculate_variance(values: &[f64]) -> f64 {
191189

192190
variance
193191
}
192+
193+
#[cfg(test)]
194+
mod tests {
195+
use super::*;
196+
use crate::types::{BBLLog, FrameStats};
197+
198+
fn create_test_log(start_time_us: u64, end_time_us: u64, total_frames: u32) -> BBLLog {
199+
BBLLog {
200+
log_number: 1,
201+
total_logs: 1,
202+
header: Default::default(),
203+
stats: FrameStats {
204+
start_time_us,
205+
end_time_us,
206+
total_frames,
207+
..Default::default()
208+
},
209+
frames: vec![],
210+
debug_frames: None,
211+
gps_coordinates: vec![],
212+
home_coordinates: vec![],
213+
event_frames: vec![],
214+
}
215+
}
216+
217+
#[test]
218+
fn test_should_skip_very_short_flight() {
219+
// Less than 5 seconds should always skip
220+
let log = create_test_log(0, 3_000_000, 4500); // 3 seconds at 1500fps
221+
let (should_skip, reason) = should_skip_export(&log, false);
222+
assert!(should_skip, "Expected to skip flight shorter than 5s");
223+
assert!(reason.contains("too short"), "Expected 'too short' reason");
224+
}
225+
226+
#[test]
227+
fn test_should_keep_five_seconds_with_good_density() {
228+
// 5 seconds at 1500fps should keep
229+
let log = create_test_log(0, 5_000_000, 7500); // 5 seconds at 1500fps
230+
let (should_skip, _) = should_skip_export(&log, false);
231+
assert!(
232+
!should_skip,
233+
"Expected to keep 5s flight with 1500fps density"
234+
);
235+
}
236+
237+
#[test]
238+
fn test_should_skip_short_flight_low_density() {
239+
// 10 seconds but only 1000fps should skip
240+
let log = create_test_log(0, 10_000_000, 10_000); // 10 seconds at 1000fps
241+
let (should_skip, reason) = should_skip_export(&log, false);
242+
assert!(
243+
should_skip,
244+
"Expected to skip 10s flight with only 1000fps density"
245+
);
246+
assert!(
247+
reason.contains("insufficient data density"),
248+
"Expected 'insufficient data density' reason"
249+
);
250+
}
251+
252+
#[test]
253+
fn test_force_export_overrides_skip() {
254+
// Even very short flight should not skip if force_export is true
255+
let log = create_test_log(0, 2_000_000, 3000); // 2 seconds
256+
let (should_skip, _) = should_skip_export(&log, true);
257+
assert!(!should_skip, "Expected force_export to prevent skip");
258+
}
259+
260+
#[test]
261+
fn test_fallback_to_frame_count() {
262+
// No duration info, but sufficient frame count should keep
263+
let log = create_test_log(0, 0, 8000); // 8000 frames, no duration
264+
let (should_skip, _) = should_skip_export(&log, false);
265+
assert!(
266+
!should_skip,
267+
"Expected to keep log with sufficient frame count"
268+
);
269+
}
270+
271+
#[test]
272+
fn test_fallback_to_frame_count_too_low() {
273+
// No duration info, insufficient frame count should skip
274+
let log = create_test_log(0, 0, 5000); // 5000 frames, no duration (below 7500 threshold)
275+
let (should_skip, reason) = should_skip_export(&log, false);
276+
assert!(should_skip, "Expected to skip log with too few frames");
277+
assert!(
278+
reason.contains("too few frames"),
279+
"Expected 'too few frames' reason"
280+
);
281+
}
282+
283+
#[test]
284+
fn test_calculate_variance() {
285+
let values = vec![1.0, 2.0, 3.0, 4.0, 5.0];
286+
let variance = calculate_variance(&values);
287+
// Expected variance: mean=3, variance=2.0
288+
assert!((variance - 2.0).abs() < 0.001);
289+
}
290+
291+
#[test]
292+
fn test_calculate_variance_single_value() {
293+
let values = vec![5.0];
294+
let variance = calculate_variance(&values);
295+
assert_eq!(variance, 0.0);
296+
}
297+
298+
#[test]
299+
fn test_calculate_variance_empty() {
300+
let values: Vec<f64> = vec![];
301+
let variance = calculate_variance(&values);
302+
assert_eq!(variance, 0.0);
303+
}
304+
}

0 commit comments

Comments
 (0)