From a1fd0288dd034016030f0d4240df78e0290abe8c Mon Sep 17 00:00:00 2001 From: "ucgmsim[bot]" <335857963+ucgmsim[bot]@users.noreply.github.com> Date: Sat, 3 Oct 2026 03:09:55 +1300 Subject: [PATCH 1/2] Fix significant-duration benchmark to feed cumulative Arias intensity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #237 ## What was wrong `significant_duration::significant_duration` expects a **cumulative Arias intensity** array (non-decreasing along each row, total in the last element) — `threshold_search` binary-searches it. The Python binding (`_significant_duration` in `src-rust/lib.rs`) always computes `cumulative_arias_intensity` first. `bench_significant_duration` in `benches/intensity_benchmarks.rs` instead passed the raw sinusoidal output of `generate_waveforms` straight into `significant_duration`. Those rows go negative and are not monotonic, so: - for stations whose last sample is negative or below `1e-10`, `threshold_search` took its early `0.0` return and did no work; - for the rest, the binary search ran over unsorted data. The benchmark therefore measured neither the real workload nor a meaningful path, and left out the cumulative-integration cost that dominates `_significant_duration` in practice. ## The fix Extracted the waveform generator and a new `significant_duration_from_waveforms` helper (integrate, then search — the same two steps the Python binding runs) into `benches/common.rs`, shared between the benchmark and a new test. `bench_significant_duration` now calls the helper inside `b.iter`, so it measures the full Ds pipeline as Python uses it, and the benchmark group is renamed to `Significant_Duration_Full_Pipeline` to say so. ## How the new test shows it `tests/significant_duration_benchmark.rs` calls the shared helper on synthetic waveforms (same generator as the benchmark) and asserts every station's Ds duration is in `(0, total_duration]`. Before the fix (i.e. with the helper calling `significant_duration` directly on raw waveforms, as the benchmark used to), this test fails — reproducing exactly the `0.0`-duration degeneracy the issue describes: ``` station 3: expected a Ds duration in (0, 9.995...], got 0; feeding raw waveforms directly into significant_duration (instead of their cumulative Arias intensity) collapses this to 0.0 for most stations ``` After computing `cumulative_arias_intensity` first, it passes. ## What to check - `benches/common.rs` is a new file included into both the bench binary (`#[path = "common.rs"] mod common;`) and the test (`#[path = "../benches/common.rs"] mod common;`) — this is the only way to get a real (non-smoke) pass/fail check on benchmark-only code, since `cargo test --benches` just runs criterion's panic-only smoke pass and doesn't check output values. - No production (`src-rust`) code changed; this is benchmark/test-only. _Cost $1.59, 15 turns._ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- benches/common.rs | 25 ++++++++++++++++ benches/intensity_benchmarks.rs | 29 ++++++++++--------- tests/significant_duration_benchmark.rs | 38 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 13 deletions(-) create mode 100644 benches/common.rs create mode 100644 tests/significant_duration_benchmark.rs diff --git a/benches/common.rs b/benches/common.rs new file mode 100644 index 00000000..a9de3bb8 --- /dev/null +++ b/benches/common.rs @@ -0,0 +1,25 @@ +use _core::{arias_intensity, significant_duration}; +use ndarray::{Array1, Array2, ArrayView2}; + +/// Generate synthetic waveform data for benchmarking (shared with tests, to +/// keep the benchmark's data generation in sync with its regression test). +pub fn generate_waveforms(stations: usize, samples: usize) -> Array2 { + // Using a simple sine wave with some noise for realistic computation + Array2::from_shape_fn((stations, samples), |(i, j)| { + 0.5 * ((j as f64 * 0.01 + i as f64).sin() + 0.1 * (j as f64 * 0.1).cos()) + }) +} + +/// The full Ds pipeline as used by the Python binding (`_significant_duration` +/// in `src-rust/lib.rs`): integrate to cumulative Arias intensity first, then +/// binary-search it for the threshold crossings. `significant_duration` +/// expects a non-decreasing cumulative intensity array, not raw waveforms. +pub fn significant_duration_from_waveforms( + waveforms: ArrayView2, + dt: f64, + low: f64, + high: f64, +) -> Array1 { + let arias = arias_intensity::cumulative_arias_intensity(waveforms, dt); + significant_duration::significant_duration(arias.view(), dt, low, high) +} diff --git a/benches/intensity_benchmarks.rs b/benches/intensity_benchmarks.rs index 8b9faa69..6137a24f 100644 --- a/benches/intensity_benchmarks.rs +++ b/benches/intensity_benchmarks.rs @@ -1,8 +1,12 @@ -use _core::{arias_intensity, cav, psa, significant_duration}; +use _core::{arias_intensity, cav, psa}; use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main}; -use ndarray::{Array2, array}; +use ndarray::array; use std::hint::black_box; +#[path = "common.rs"] +mod common; +use common::generate_waveforms; + // Configuration constants for test scenarios const SAMPLING_RATE: f64 = 0.005; // 200 Hz const STATION_COUNTS: &[usize] = &[1, 10, 100, 1000, 10000]; @@ -16,14 +20,6 @@ const SAMPLE_LENGTHS: &[usize] = &[ const PSA_PERIODS: &[f64] = &[1.0]; // Should be independent of pSA period, but added here for safety in the future const DAMPING: f64 = 0.05; -/// Generate synthetic waveform data for benchmarking -fn generate_waveforms(stations: usize, samples: usize) -> Array2 { - // Using a simple sine wave with some noise for realistic computation - Array2::from_shape_fn((stations, samples), |(i, j)| { - 0.5 * ((j as f64 * 0.01 + i as f64).sin() + 0.1 * (j as f64 * 0.1).cos()) - }) -} - /// Benchmark CAV (Cumulative Absolute Velocity) calculations fn bench_cav(c: &mut Criterion) { let mut group = c.benchmark_group("CAV"); @@ -93,9 +89,16 @@ fn bench_cumulative_arias(c: &mut Criterion) { group.finish(); } -/// Benchmark Significant Duration calculations +/// Benchmark Significant Duration calculations. +/// +/// This measures the full Ds pipeline as the Python binding +/// (`_significant_duration` in `src-rust/lib.rs`) uses it: integrating raw +/// waveforms to cumulative Arias intensity, then binary-searching that for +/// the threshold crossings. `significant_duration` itself requires a +/// non-decreasing cumulative intensity array, so the integration step can't +/// be skipped without benchmarking an unrepresentative, degenerate input. fn bench_significant_duration(c: &mut Criterion) { - let mut group = c.benchmark_group("Significant_Duration"); + let mut group = c.benchmark_group("Significant_Duration_Full_Pipeline"); for &stations in STATION_COUNTS { for &samples in SAMPLE_LENGTHS { @@ -107,7 +110,7 @@ fn bench_significant_duration(c: &mut Criterion) { group.bench_with_input(BenchmarkId::new("Sequential", ¶m), &view, |b, &v| { b.iter(|| { - significant_duration::significant_duration( + common::significant_duration_from_waveforms( black_box(v), black_box(SAMPLING_RATE), 0.05, diff --git a/tests/significant_duration_benchmark.rs b/tests/significant_duration_benchmark.rs new file mode 100644 index 00000000..da579ebe --- /dev/null +++ b/tests/significant_duration_benchmark.rs @@ -0,0 +1,38 @@ +//! Regression test for the `Significant_Duration` criterion benchmark +//! (see `benches/intensity_benchmarks.rs` / `benches/common.rs`). +//! +//! `significant_duration::significant_duration` expects a cumulative Arias +//! intensity array (non-decreasing, total in the last element), the same way +//! the Python binding `_significant_duration` (src-rust/lib.rs) feeds it. The +//! benchmark used to pass raw, sinusoidal waveforms straight in instead, +//! which are not monotonic and go negative, so `threshold_search` either took +//! its early `0.0` return or binary-searched unsorted data. This test exercises +//! the shared helper the benchmark now uses and checks it reports a sane, +//! bounded Ds duration for realistic waveform data, not the degenerate `0.0` +//! that feeding raw waveforms directly into `significant_duration` produces. + +#[path = "../benches/common.rs"] +mod common; + +const SAMPLING_RATE: f64 = 0.005; + +#[test] +fn significant_duration_from_waveforms_matches_full_pipeline() { + let stations = 10; + let samples = 2000; + let waveforms = common::generate_waveforms(stations, samples); + let total_duration = (samples - 1) as f64 * SAMPLING_RATE; + + let ds = + common::significant_duration_from_waveforms(waveforms.view(), SAMPLING_RATE, 0.05, 0.95); + + assert_eq!(ds.len(), stations); + for (station, &duration) in ds.iter().enumerate() { + assert!( + duration > 0.0 && duration <= total_duration, + "station {station}: expected a Ds duration in (0, {total_duration}], got {duration}; \ + feeding raw waveforms directly into significant_duration (instead of their cumulative \ + Arias intensity) collapses this to 0.0 for most stations", + ); + } +} From ed7d0b8114b99fba52c0f45afcad62c273aff18a Mon Sep 17 00:00:00 2001 From: Jake Faulkner Date: Sat, 3 Oct 2026 08:27:34 +1300 Subject: [PATCH 2/2] Delete tests/significant_duration_benchmark.rs --- tests/significant_duration_benchmark.rs | 38 ------------------------- 1 file changed, 38 deletions(-) delete mode 100644 tests/significant_duration_benchmark.rs diff --git a/tests/significant_duration_benchmark.rs b/tests/significant_duration_benchmark.rs deleted file mode 100644 index da579ebe..00000000 --- a/tests/significant_duration_benchmark.rs +++ /dev/null @@ -1,38 +0,0 @@ -//! Regression test for the `Significant_Duration` criterion benchmark -//! (see `benches/intensity_benchmarks.rs` / `benches/common.rs`). -//! -//! `significant_duration::significant_duration` expects a cumulative Arias -//! intensity array (non-decreasing, total in the last element), the same way -//! the Python binding `_significant_duration` (src-rust/lib.rs) feeds it. The -//! benchmark used to pass raw, sinusoidal waveforms straight in instead, -//! which are not monotonic and go negative, so `threshold_search` either took -//! its early `0.0` return or binary-searched unsorted data. This test exercises -//! the shared helper the benchmark now uses and checks it reports a sane, -//! bounded Ds duration for realistic waveform data, not the degenerate `0.0` -//! that feeding raw waveforms directly into `significant_duration` produces. - -#[path = "../benches/common.rs"] -mod common; - -const SAMPLING_RATE: f64 = 0.005; - -#[test] -fn significant_duration_from_waveforms_matches_full_pipeline() { - let stations = 10; - let samples = 2000; - let waveforms = common::generate_waveforms(stations, samples); - let total_duration = (samples - 1) as f64 * SAMPLING_RATE; - - let ds = - common::significant_duration_from_waveforms(waveforms.view(), SAMPLING_RATE, 0.05, 0.95); - - assert_eq!(ds.len(), stations); - for (station, &duration) in ds.iter().enumerate() { - assert!( - duration > 0.0 && duration <= total_duration, - "station {station}: expected a Ds duration in (0, {total_duration}], got {duration}; \ - feeding raw waveforms directly into significant_duration (instead of their cumulative \ - Arias intensity) collapses this to 0.0 for most stations", - ); - } -}