Skip to content

fix: preserve topology and public error invariants #363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acgetchell merged 4 commits into from
May 8, 2026
Merged

fix: preserve topology and public error invariants #363

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54c96d8
fix: preserve topology and public error invariants
acgetchell May 7, 2026
e68d878
fix: preserve topology and public error invariants
acgetchell May 8, 2026
7a560b2
fix!: make duplicate-cell removal transactional
acgetchell May 8, 2026
ddbf4fb
fix: isolate duplicate-cell cleanup generation state
acgetchell May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
282 changes: 174 additions & 108 deletions benches/ci_performance_suite.rs
View file
Open in desktop

Large diffs are not rendered by default.

141 changes: 119 additions & 22 deletions benches/circumsphere_containment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@ use delaunay::prelude::generators::generate_random_points_seeded;
use delaunay::prelude::query::*;
use std::hint::black_box;

fn abort_benchmark(message: impl std::fmt::Display) -> ! {
#[cfg(not(feature = "bench-logging"))]
let _ = &message;
#[cfg(feature = "bench-logging")]
tracing::error!("{message}");
std::process::exit(1);
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same silent-exit issue: abort_benchmark emits nothing without bench-logging

let _ = &message discards the error context and process::exit(1) fires silently. Same root cause as the equivalent function in cold_path_predicates.rs — fatal benchmark failures produce an opaque exit code 1 with no output unless the feature is explicitly enabled.

🛠️ Proposed fix 
 fn abort_benchmark(message: impl std::fmt::Display) -> ! {
-    #[cfg(not(feature = "bench-logging"))]
-    let _ = &message;
+    #[cfg(not(feature = "bench-logging"))]
+    {
+        let _ = std::io::Write::write_fmt(
+            &mut std::io::stderr(),
+            format_args!("{message}\n"),
+        );
+    }
     #[cfg(feature = "bench-logging")]
     tracing::error!("{message}");
     std::process::exit(1);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn abort_benchmark(message: impl std::fmt::Display) -> ! {
#[cfg(not(feature = "bench-logging"))]
let _ = &message;
#[cfg(feature = "bench-logging")]
tracing::error!("{message}");
std::process::exit(1);
}
fn abort_benchmark(message: impl std::fmt::Display) -> ! {
#[cfg(not(feature = "bench-logging"))]
{
let _ = std::io::Write::write_fmt(
&mut std::io::stderr(),
format_args!("{message}\n"),
);
}
#[cfg(feature = "bench-logging")]
tracing::error!("{message}");
std::process::exit(1);
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@benches/circumsphere_containment.rs` around lines 21 - 27, The
abort_benchmark function currently discards the message when the bench-logging
feature is disabled (letting the error silence and then calling process::exit),
so change abort_benchmark to always emit the message to a visible sink before
exiting: stop using "let _ = &message", and instead write the formatted message
to stderr (e.g., via eprintln! or writing to std::io::stderr()) unconditionally,
while keeping the tracing::error branch when the feature is enabled; ensure the
function signature and call sites (abort_benchmark) remain the same and that the
error text is flushed/visible before calling std::process::exit(1).


fn bench_result<T, E: std::fmt::Display>(result: Result<T, E>, context: &str) -> T {
match result {
Ok(value) => value,
Err(error) => abort_benchmark(format_args!("{context}: {error}")),
}
}

fn bench_option<T>(option: Option<T>, context: &str) -> T {
option.unwrap_or_else(|| abort_benchmark(context))
}

/// Generate a standard D-dimensional simplex (D+1 vertices)
///
/// Creates a simplex with vertices at:
Expand All @@ -41,17 +60,19 @@ fn standard_simplex<const D: usize>() -> Vec<Point<f64, D>> {

/// Generate a random 3D simplex (tetrahedron) for benchmarking using seeded generation
fn generate_random_simplex_3d(seed: u64) -> Vec<Point<f64, 3>> {
generate_random_points_seeded(4, (-10.0, 10.0), seed)
.expect("Failed to generate random simplex points")
bench_result(
generate_random_points_seeded(4, (-10.0, 10.0), seed),
"failed to generate random simplex points",
)
}

/// Generate a random 3D test point using seeded generation
fn generate_random_test_point_3d(seed: u64) -> Point<f64, 3> {
generate_random_points_seeded(1, (-5.0, 5.0), seed)
.expect("Failed to generate random test point")
.into_iter()
.next()
.expect("Expected exactly one test point")
let points = bench_result(
generate_random_points_seeded(1, (-5.0, 5.0), seed),
"failed to generate random test point",
);
bench_option(points.into_iter().next(), "expected exactly one test point")
}

/// Benchmark with many random queries
Expand All @@ -60,33 +81,51 @@ fn benchmark_random_queries(c: &mut Criterion) {
let simplex_points = generate_random_simplex_3d(42);

// Generate many test points using seeded generation for reproducible results
let test_points = generate_random_points_seeded(1000, (-5.0, 5.0), 123)
.expect("Failed to generate random test points");
let test_points = bench_result(
generate_random_points_seeded(1000, (-5.0, 5.0), 123),
"failed to generate random test points",
);

c.bench_function("random/insphere_1000_queries", |b| {
b.iter(|| {
for test_point in &test_points {
black_box(insphere(black_box(&simplex_points), black_box(*test_point)).unwrap());
let result = match insphere(black_box(&simplex_points), black_box(*test_point)) {
Ok(value) => value,
Err(error) => abort_benchmark(format_args!("insphere query failed: {error}")),
};
black_box(result);
}
});
});

c.bench_function("random/insphere_distance_1000_queries", |b| {
b.iter(|| {
for test_point in &test_points {
black_box(
insphere_distance(black_box(&simplex_points), black_box(*test_point)).unwrap(),
);
let result =
match insphere_distance(black_box(&simplex_points), black_box(*test_point)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"insphere_distance query failed: {error}"
));
}
};
black_box(result);
}
});
});

c.bench_function("random/insphere_lifted_1000_queries", |b| {
b.iter(|| {
for test_point in &test_points {
black_box(
insphere_lifted(black_box(&simplex_points), black_box(*test_point)).unwrap(),
);
let result =
match insphere_lifted(black_box(&simplex_points), black_box(*test_point)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!("insphere_lifted query failed: {error}"));
}
};
black_box(result);
}
});
});
Expand All @@ -96,13 +135,41 @@ fn benchmark_random_queries(c: &mut Criterion) {
macro_rules! bench_simplex {
($c:ident, $dim:literal, $simplex:expr, $pt:expr) => {{
$c.bench_function(concat!($dim, "d/insphere"), |b| {
b.iter(|| black_box(insphere(black_box(&$simplex), black_box($pt)).unwrap()))
b.iter(|| {
let result = match insphere(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!("insphere benchmark query failed: {error}"));
}
};
black_box(result)
})
});
$c.bench_function(concat!($dim, "d/insphere_distance"), |b| {
b.iter(|| black_box(insphere_distance(black_box(&$simplex), black_box($pt)).unwrap()))
b.iter(|| {
let result = match insphere_distance(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"insphere_distance benchmark query failed: {error}"
));
}
};
black_box(result)
})
});
$c.bench_function(concat!($dim, "d/insphere_lifted"), |b| {
b.iter(|| black_box(insphere_lifted(black_box(&$simplex), black_box($pt)).unwrap()))
b.iter(|| {
let result = match insphere_lifted(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"insphere_lifted benchmark query failed: {error}"
));
}
};
black_box(result)
})
});
}};
}
Expand All @@ -112,18 +179,48 @@ macro_rules! bench_edge_case {
($c:ident, $dim:literal, $case:literal, $simplex:expr, $pt:expr) => {{
$c.bench_function(
concat!("edge_cases_", $dim, "d/", $case, "_insphere"),
|b| b.iter(|| black_box(insphere(black_box(&$simplex), black_box($pt)).unwrap())),
|b| {
b.iter(|| {
let result = match insphere(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"edge-case insphere benchmark query failed: {error}"
));
}
};
black_box(result)
})
},
);
$c.bench_function(
concat!("edge_cases_", $dim, "d/", $case, "_distance"),
|b| {
b.iter(|| {
black_box(insphere_distance(black_box(&$simplex), black_box($pt)).unwrap())
let result = match insphere_distance(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"edge-case insphere_distance benchmark query failed: {error}"
));
}
};
black_box(result)
})
},
);
$c.bench_function(concat!("edge_cases_", $dim, "d/", $case, "_lifted"), |b| {
b.iter(|| black_box(insphere_lifted(black_box(&$simplex), black_box($pt)).unwrap()))
b.iter(|| {
let result = match insphere_lifted(black_box(&$simplex), black_box($pt)) {
Ok(value) => value,
Err(error) => {
abort_benchmark(format_args!(
"edge-case insphere_lifted benchmark query failed: {error}"
));
}
};
black_box(result)
})
});
}};
}
Expand Down
54 changes: 48 additions & 6 deletions benches/cold_path_predicates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,36 @@ use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_m
use delaunay::prelude::generators::generate_random_points_seeded;
use delaunay::prelude::query::*;
use std::hint::black_box;
#[cfg(feature = "bench-logging")]
use std::sync::Once;

#[cfg(feature = "bench-logging")]
fn init_tracing() {
static INIT: Once = Once::new();
INIT.call_once(|| {
let filter = tracing_subscriber::EnvFilter::try_from_default_env()
.unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("error"));
let _ = tracing_subscriber::fmt().with_env_filter(filter).try_init();
});
}

fn abort_benchmark<E: std::fmt::Display>(context: &str, error: E) -> ! {
#[cfg(not(feature = "bench-logging"))]
let _ = (context, &error);
#[cfg(feature = "bench-logging")]
{
init_tracing();
tracing::error!(context = %context, error = %error);
}
std::process::exit(1);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

fn bench_result<T, E: std::fmt::Display>(result: Result<T, E>, context: &str) -> T {
match result {
Ok(value) => value,
Err(error) => abort_benchmark(context, error),
}
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
/// Deterministic seed for query-point generation in the hot path.
const HOT_SEED: u64 = 0xC01D_BEEF_0000_CAFE_u64;
Expand Down Expand Up @@ -65,8 +95,10 @@ fn standard_simplex<const D: usize>() -> Vec<Point<f64, D>> {
/// Uses the range `[-10, 10]` against a unit simplex so that the Shewchuk
/// errbound comfortably resolves the sign in Stage 1.
fn hot_queries<const D: usize>() -> Vec<Point<f64, D>> {
generate_random_points_seeded(HOT_QUERIES, (-10.0, 10.0), HOT_SEED)
.expect("failed to generate hot-path query points")
bench_result(
generate_random_points_seeded(HOT_QUERIES, (-10.0, 10.0), HOT_SEED),
"failed to generate hot-path query points",
)
}

/// Generate near-boundary query points for dimension `D`.
Expand All @@ -77,21 +109,31 @@ fn near_boundary_queries<const D: usize>() -> Vec<Point<f64, D>> {
// Centered near the circumsphere radius of the standard simplex (~0.5 for
// the D = 3 unit case); the exact value is unimportant — we just want a
// high rate of errbound-ambiguous inputs.
generate_random_points_seeded(NEAR_BOUNDARY_QUERIES, (0.40, 0.60), NEAR_BOUNDARY_SEED)
.expect("failed to generate near-boundary query points")
bench_result(
generate_random_points_seeded(NEAR_BOUNDARY_QUERIES, (0.40, 0.60), NEAR_BOUNDARY_SEED),
"failed to generate near-boundary query points",
)
}

/// Run `insphere` across `queries` against `simplex`, black-boxing each result.
fn run_insphere<const D: usize>(simplex: &[Point<f64, D>], queries: &[Point<f64, D>]) {
for q in queries {
black_box(insphere(black_box(simplex), black_box(*q)).unwrap());
let result = match insphere(black_box(simplex), black_box(*q)) {
Ok(value) => value,
Err(error) => abort_benchmark("insphere query failed", error),
};
black_box(result);
}
}

/// Run `insphere_lifted` across `queries` against `simplex`, black-boxing each result.
fn run_insphere_lifted<const D: usize>(simplex: &[Point<f64, D>], queries: &[Point<f64, D>]) {
for q in queries {
black_box(insphere_lifted(black_box(simplex), black_box(*q)).unwrap());
let result = match insphere_lifted(black_box(simplex), black_box(*q)) {
Ok(value) => value,
Err(error) => abort_benchmark("insphere_lifted query failed", error),
};
black_box(result);
}
}

Expand Down
Loading
Loading