Skip to content

Commit 67c8c88

Browse files
Deduplicate number_of_pixels and fix doc/naming inconsistencies
R1: three byte-identical number_of_pixels helpers (label_accumulation, feature_accumulation, node_label_projection) — the feature_accumulation copy was dead. Add detail::number_of_elements to detail/grid.hxx and route the two live call sites through it; drop the dead copy. R9: drop the redundant nb::cast wrappers inside nb::make_tuple in relabel_sequential_t (make_tuple already casts its arguments). Docs/naming: - AGENTS.md: correct the union-find path (util/union_find.hxx, namespace bioimage_cpp::util, not detail/) and list number_of_elements under grid.hxx. - agglomeration/detail.hxx: the rekey comment described the opposite of the code; reword to explain that a kRejectEdge'd edge is deliberately left out of the heap. - filters: the public kwarg was window_ratio while the Python API documents window_size; align the nb::arg keyword and the validation message to window_size (calls are positional, so behavior is unchanged). - feature_accumulation: note that float feature values are bit-reproducible only for a fixed thread count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 1946cb4 commit 67c8c88

8 files changed

Lines changed: 41 additions & 56 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,10 @@ Required in the environment: `scikit-build-core`, `cmake>=3.21`, `ninja`, `nanob
2424

2525
Before introducing union-finds, priority queues, edge hashing, stride math, threading helpers, or label relabeling, check `include/bioimage_cpp/detail/`:
2626

27-
- `detail/union_find.hxx``UnionFind` (path compression + union by rank). `find`, `merge`, `merge_to`, `unite_roots`.
27+
- `util/union_find.hxx``UnionFind` (namespace `bioimage_cpp::util`, not `detail`; path compression + union by rank). `find`, `merge`, `merge_to`, `unite_roots`.
2828
- `detail/indexed_heap.hxx` — addressable max-heap with mutable priorities. `DenseIndexedHeap` (vector-backed locator, integer keys in `[0, N)`) and `SparseIndexedHeap` (hashmap-backed, arbitrary keys).
2929
- `detail/edge_hash.hxx``Edge`, `edge_key`, `EdgeHash` for hashing unordered node pairs.
30-
- `detail/grid.hxx``c_order_strides`, `valid_offset_target`, `is_valid_grid_edge` for row-major grid offsets.
30+
- `detail/grid.hxx``number_of_elements`, `c_order_strides`, `valid_offset_target`, `is_valid_grid_edge` for row-major grid shapes/offsets.
3131
- `detail/relabel.hxx``dense_relabel` to map arbitrary labels onto `[0, k)` preserving first-occurrence order.
3232
- `detail/threading.hxx``normalize_thread_count`, `parallel_for_chunks(n_threads, n_items, chunk)`.
3333

include/bioimage_cpp/detail/grid.hxx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,16 @@
66

77
namespace bioimage_cpp::detail {
88

9+
// Total number of elements in a row-major array of the given shape (the product
10+
// of the per-axis extents). Shape entries are assumed non-negative.
11+
inline std::size_t number_of_elements(const std::vector<std::ptrdiff_t> &shape) {
12+
std::size_t total = 1;
13+
for (const auto extent : shape) {
14+
total *= static_cast<std::size_t>(extent);
15+
}
16+
return total;
17+
}
18+
919
// C-order strides for a row-major array of the given shape, in units of array
1020
// elements (not bytes). The innermost (last) axis has stride 1.
1121
inline std::vector<std::ptrdiff_t> c_order_strides(const std::vector<std::ptrdiff_t> &shape) {

include/bioimage_cpp/graph/agglomeration/detail.hxx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,9 @@ inline std::size_t agglo_merge_dynamic_nodes(
9393
if (new_priority != current_priority) {
9494
edge.weight = new_priority;
9595
// The edge may have been previously popped via kRejectEdge
96-
// (GASP cannot-link), in which case it is no longer in the
97-
// heap. Use push_or_change to handle both cases.
96+
// (GASP cannot-link). In that case it must stay out of the heap
97+
// (re-adding it would let it be re-popped and merged), so only
98+
// update the priority when the edge is still present.
9899
if (heap.contains(removed_edge_id)) {
99100
heap.change(removed_edge_id, new_priority);
100101
}

include/bioimage_cpp/graph/feature_accumulation.hxx

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -88,15 +88,6 @@ inline double percentile(std::vector<double> &values, const double percentile) {
8888
return (1.0 - weight) * lower + weight * upper;
8989
}
9090

91-
inline std::size_t number_of_pixels(const std::vector<std::ptrdiff_t> &shape) {
92-
return static_cast<std::size_t>(std::accumulate(
93-
shape.begin(),
94-
shape.end(),
95-
std::ptrdiff_t{1},
96-
[](const std::ptrdiff_t a, const std::ptrdiff_t b) { return a * b; }
97-
));
98-
}
99-
10091
inline void require_same_spatial_shape(
10192
const std::vector<std::ptrdiff_t> &labels_shape,
10293
const std::vector<std::ptrdiff_t> &data_shape,
@@ -370,6 +361,11 @@ void scan_affinity_3d_chunk(
370361
}
371362
}
372363

364+
// Combine per-thread partial statistics. count/min/max/percentile results are
365+
// exact regardless of thread count; the floating-point sum / sum-of-squares
366+
// (and hence mean / std) can differ in the last ULP between thread counts,
367+
// because each thread sums a different subset of pixels. Float feature values
368+
// are therefore bit-reproducible only for a fixed thread count.
373369
template <class Stats>
374370
std::vector<Stats> merge_stats(
375371
std::vector<std::vector<Stats>> &per_thread_stats,

include/bioimage_cpp/graph/label_accumulation.hxx

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
#pragma once
22

33
#include "bioimage_cpp/array_view.hxx"
4+
#include "bioimage_cpp/detail/grid.hxx"
45
#include "bioimage_cpp/detail/threading.hxx"
56
#include "bioimage_cpp/graph/node_label_projection.hxx"
67
#include "bioimage_cpp/graph/region_adjacency_graph.hxx"
78

89
#include <cstddef>
910
#include <cstdint>
10-
#include <numeric>
1111
#include <stdexcept>
1212
#include <string>
1313
#include <unordered_map>
@@ -17,15 +17,6 @@ namespace bioimage_cpp::graph {
1717

1818
namespace detail_label_accumulation {
1919

20-
inline std::size_t number_of_pixels(const std::vector<std::ptrdiff_t> &shape) {
21-
return static_cast<std::size_t>(std::accumulate(
22-
shape.begin(),
23-
shape.end(),
24-
std::ptrdiff_t{1},
25-
[](const std::ptrdiff_t a, const std::ptrdiff_t b) { return a * b; }
26-
));
27-
}
28-
2920
// Combined (node, other) histogram key. A single map per thread keyed by this
3021
// pair avoids the n_threads * n_nodes map-of-maps (one std::unordered_map per
3122
// node per thread), which dominates allocation for RAGs with many nodes.
@@ -104,7 +95,7 @@ void accumulate_labels(
10495
throw std::invalid_argument("labels contain a node id outside the rag");
10596
}
10697

107-
const auto n_pixels = detail_label_accumulation::number_of_pixels(labels.shape);
98+
const auto n_pixels = bioimage_cpp::detail::number_of_elements(labels.shape);
10899
const auto n_threads = detail::normalize_thread_count(number_of_threads, n_pixels);
109100

110101
std::vector<detail_label_accumulation::NodeOtherHistogram<OtherT>> per_thread(n_threads);

include/bioimage_cpp/graph/node_label_projection.hxx

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
#pragma once
22

33
#include "bioimage_cpp/array_view.hxx"
4+
#include "bioimage_cpp/detail/grid.hxx"
45
#include "bioimage_cpp/detail/threading.hxx"
56
#include "bioimage_cpp/graph/region_adjacency_graph.hxx"
67

78
#include <cstddef>
89
#include <cstdint>
9-
#include <numeric>
1010
#include <stdexcept>
1111
#include <string>
1212
#include <vector>
@@ -15,15 +15,6 @@ namespace bioimage_cpp::graph {
1515

1616
namespace detail_projection {
1717

18-
inline std::size_t number_of_pixels(const std::vector<std::ptrdiff_t> &shape) {
19-
return static_cast<std::size_t>(std::accumulate(
20-
shape.begin(),
21-
shape.end(),
22-
std::ptrdiff_t{1},
23-
[](const std::ptrdiff_t a, const std::ptrdiff_t b) { return a * b; }
24-
));
25-
}
26-
2718
inline void require_rag_shape_matches_labels(
2819
const RegionAdjacencyGraph &rag,
2920
const std::vector<std::ptrdiff_t> &labels_shape
@@ -69,7 +60,7 @@ void project_node_labels_to_pixels(
6960
throw std::invalid_argument("labels contain a node id outside node_labels");
7061
}
7162

72-
const auto n_pixels = detail_projection::number_of_pixels(labels.shape);
63+
const auto n_pixels = bioimage_cpp::detail::number_of_elements(labels.shape);
7364
const auto n_threads = detail::normalize_thread_count(number_of_threads, n_pixels);
7465
bioimage_cpp::detail::parallel_for_chunks(
7566
n_threads,

src/bindings/filters.cxx

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,11 @@ void require_order(int order, const char *name, const char *function) {
4343
}
4444
}
4545

46-
void require_non_negative_window(double window_ratio, const char *function) {
47-
if (window_ratio < 0.0) {
46+
void require_non_negative_window(double window_size, const char *function) {
47+
if (window_size < 0.0) {
4848
throw std::invalid_argument(
49-
std::string(function) + ": window_ratio must be >= 0 (0 selects the "
50-
"default), got " + std::to_string(window_ratio)
49+
std::string(function) + ": window_size must be >= 0 (0 selects the "
50+
"default), got " + std::to_string(window_size)
5151
);
5252
}
5353
}
@@ -476,69 +476,69 @@ void bind_filters(nb::module_ &m) {
476476
m.def(
477477
"_gaussian_smoothing_2d_float32", &gaussian_smoothing_2d,
478478
nb::arg("image"), nb::arg("sigma_y"), nb::arg("sigma_x"),
479-
nb::arg("window_ratio") = 0.0,
479+
nb::arg("window_size") = 0.0,
480480
"2D Gaussian smoothing on a float32 (ny, nx) image with anisotropic sigma."
481481
);
482482
m.def(
483483
"_gaussian_smoothing_3d_float32", &gaussian_smoothing_3d,
484484
nb::arg("image"),
485485
nb::arg("sigma_z"), nb::arg("sigma_y"), nb::arg("sigma_x"),
486-
nb::arg("window_ratio") = 0.0,
486+
nb::arg("window_size") = 0.0,
487487
"3D Gaussian smoothing on a float32 (nz, ny, nx) image with anisotropic sigma."
488488
);
489489
m.def(
490490
"_gaussian_derivative_2d_float32", &gaussian_derivative_2d,
491491
nb::arg("image"), nb::arg("sigma_y"), nb::arg("sigma_x"),
492492
nb::arg("order_y"), nb::arg("order_x"),
493-
nb::arg("window_ratio") = 0.0,
493+
nb::arg("window_size") = 0.0,
494494
"2D Gaussian derivative on a float32 (ny, nx) image with per-axis order."
495495
);
496496
m.def(
497497
"_gaussian_derivative_3d_float32", &gaussian_derivative_3d,
498498
nb::arg("image"),
499499
nb::arg("sigma_z"), nb::arg("sigma_y"), nb::arg("sigma_x"),
500500
nb::arg("order_z"), nb::arg("order_y"), nb::arg("order_x"),
501-
nb::arg("window_ratio") = 0.0,
501+
nb::arg("window_size") = 0.0,
502502
"3D Gaussian derivative on a float32 (nz, ny, nx) image with per-axis order."
503503
);
504504
m.def(
505505
"_gaussian_gradient_magnitude_2d_float32", &gaussian_gradient_magnitude_2d,
506506
nb::arg("image"), nb::arg("sigma_y"), nb::arg("sigma_x"),
507-
nb::arg("window_ratio") = 0.0,
507+
nb::arg("window_size") = 0.0,
508508
"Gradient magnitude of a Gaussian-smoothed 2D float32 image."
509509
);
510510
m.def(
511511
"_gaussian_gradient_magnitude_3d_float32", &gaussian_gradient_magnitude_3d,
512512
nb::arg("image"),
513513
nb::arg("sigma_z"), nb::arg("sigma_y"), nb::arg("sigma_x"),
514-
nb::arg("window_ratio") = 0.0,
514+
nb::arg("window_size") = 0.0,
515515
"Gradient magnitude of a Gaussian-smoothed 3D float32 image."
516516
);
517517
m.def(
518518
"_laplacian_of_gaussian_2d_float32", &laplacian_of_gaussian_2d,
519519
nb::arg("image"), nb::arg("sigma_y"), nb::arg("sigma_x"),
520-
nb::arg("window_ratio") = 0.0,
520+
nb::arg("window_size") = 0.0,
521521
"Laplacian of Gaussian on a 2D float32 image."
522522
);
523523
m.def(
524524
"_laplacian_of_gaussian_3d_float32", &laplacian_of_gaussian_3d,
525525
nb::arg("image"),
526526
nb::arg("sigma_z"), nb::arg("sigma_y"), nb::arg("sigma_x"),
527-
nb::arg("window_ratio") = 0.0,
527+
nb::arg("window_size") = 0.0,
528528
"Laplacian of Gaussian on a 3D float32 image."
529529
);
530530
m.def(
531531
"_hessian_of_gaussian_eigenvalues_2d_float32", &hessian_of_gaussian_eigenvalues_2d,
532532
nb::arg("image"), nb::arg("sigma_y"), nb::arg("sigma_x"),
533-
nb::arg("window_ratio") = 0.0,
533+
nb::arg("window_size") = 0.0,
534534
"Eigenvalues of the Hessian of Gaussian on a 2D float32 image. "
535535
"Output shape: (ny, nx, 2), sorted descending along the trailing axis."
536536
);
537537
m.def(
538538
"_hessian_of_gaussian_eigenvalues_3d_float32", &hessian_of_gaussian_eigenvalues_3d,
539539
nb::arg("image"),
540540
nb::arg("sigma_z"), nb::arg("sigma_y"), nb::arg("sigma_x"),
541-
nb::arg("window_ratio") = 0.0,
541+
nb::arg("window_size") = 0.0,
542542
"Eigenvalues of the Hessian of Gaussian on a 3D float32 image. "
543543
"Output shape: (nz, ny, nx, 3), sorted descending along the trailing axis."
544544
);
@@ -547,7 +547,7 @@ void bind_filters(nb::module_ &m) {
547547
nb::arg("image"),
548548
nb::arg("sigma_inner_y"), nb::arg("sigma_inner_x"),
549549
nb::arg("sigma_outer_y"), nb::arg("sigma_outer_x"),
550-
nb::arg("window_ratio") = 0.0,
550+
nb::arg("window_size") = 0.0,
551551
"Eigenvalues of the structure tensor on a 2D float32 image. "
552552
"Output shape: (ny, nx, 2), sorted descending along the trailing axis."
553553
);
@@ -556,7 +556,7 @@ void bind_filters(nb::module_ &m) {
556556
nb::arg("image"),
557557
nb::arg("sigma_inner_z"), nb::arg("sigma_inner_y"), nb::arg("sigma_inner_x"),
558558
nb::arg("sigma_outer_z"), nb::arg("sigma_outer_y"), nb::arg("sigma_outer_x"),
559-
nb::arg("window_ratio") = 0.0,
559+
nb::arg("window_size") = 0.0,
560560
"Eigenvalues of the structure tensor on a 3D float32 image. "
561561
"Output shape: (nz, ny, nx, 3), sorted descending along the trailing axis."
562562
);

src/bindings/segmentation.cxx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -392,11 +392,7 @@ nb::tuple relabel_sequential_t(
392392
inverse_owner
393393
);
394394

395-
return nb::make_tuple(
396-
nb::cast(relabeled_array),
397-
nb::cast(forward_array),
398-
nb::cast(inverse_array)
399-
);
395+
return nb::make_tuple(relabeled_array, forward_array, inverse_array);
400396
}
401397

402398
template <class InT>

0 commit comments

Comments
 (0)