Skip to content

feat: Suzuki and Abe Contour finding implementation#767

Open
zeel991 wants to merge 11 commits intokornia:mainfrom
zeel991:contour
Open

feat: Suzuki and Abe Contour finding implementation#767
zeel991 wants to merge 11 commits intokornia:mainfrom
zeel991:contour

Conversation

@zeel991
Copy link
Contributor

@zeel991 zeel991 commented Feb 26, 2026

📝 Description

⚠️ Issue Link Required: #169

Fixes/Relates to: #169

Important:

  • Ensure you are assigned to the linked issue before submitting this PR
  • This PR should strictly implement what the linked issue describes
  • Do not include changes beyond the scope of the linked issue

🛠️ Changes Made

  • New Contour Finding Module (kornia-imgproc): Full implementation of the Suzuki & Abe (1985) border-following algorithm with find_contours and a reusable FindContoursExecutor.
  • SWAR Scanning: Raster scanner uses u64 word reads to skip 4 zero pixels at a time, reducing per-pixel overhead in sparse/background regions.
  • Single-Pass Initialization: Binarization and virtual padding are combined into one pass for better L1/L2 cache efficiency.
  • Full Hierarchy Support: External, List, CComp, and Tree retrieval modes with correct parent-child nesting. Simple and None approximation modes also supported.

🧪 How Was This Tested?

  • Unit Tests: test_simple_square_no_approx, test_simple_square_simple_approx, test_hollow_square_external_vs_list, test_isolated_pixel, test_u_shape, test_no_empty_contour_simple_approx, test_many_contours_no_overflow, test_executor_reuse_identical
  • Performance/Edge Cases: Isolated pixels, hollow shapes, U-shapes, 63-contour stress test, executor buffer reuse across calls.

🕵️ AI Usage Disclosure

Check one of the following:

  • 🟢 No AI used.
  • 🟡 AI-assisted: I used AI for boilerplate/refactoring but have manually reviewed and tested every line.
  • 🔴 AI-generated: (Note: These PRs may be subject to stricter scrutiny or immediate closure if the logic is not explained).

🚦 Checklist

  • I am assigned to the linked issue (required before PR submission)
  • The linked issue has been approved by a maintainer
  • This PR strictly implements what the linked issue describes (no scope creep)
  • I have performed a self-review of my code (no "ghost" variables or hallucinations).
  • My code follows the existing style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • (Optional) I have attached screenshots/recordings for UI changes.

💭 Additional Context

Add any other context or screenshots about the pull request here.

@zeel991 zeel991 marked this pull request as ready for review March 2, 2026 10:30
Copilot AI review requested due to automatic review settings March 2, 2026 10:30
@qodo-code-review
Copy link
Contributor

Review Summary by Qodo

Implement Suzuki & Abe contour finding with SWAR optimization

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Implements Suzuki & Abe (1985) border-following algorithm for contour detection
• Supports four retrieval modes (External, List, CComp, Tree) with full hierarchy
• Includes two approximation modes (None, Simple) for contour point compression
• Optimizes scanning with SWAR (Single-Word-At-a-time Read) to skip zero pixels
• Provides reusable FindContoursExecutor for repeated processing without allocation
• Adds comprehensive benchmarks comparing against OpenCV implementation
Diagram
flowchart LR
  Input["Input Image<br/>u8 pixels"]
  Binarize["Binarize &<br/>Virtual Pad"]
  Scan["Raster Scan<br/>with SWAR"]
  Trace["Trace Border<br/>Suzuki-Abe"]
  Approx["Approximate<br/>None/Simple"]
  Filter["Filter by<br/>Retrieval Mode"]
  Output["ContoursResult<br/>contours + hierarchy"]
  
  Input --> Binarize
  Binarize --> Scan
  Scan --> Trace
  Trace --> Approx
  Approx --> Filter
  Filter --> Output
Loading

Grey Divider

File Changes

1. crates/kornia-imgproc/src/contours.rs ✨ Enhancement +820/-0

Suzuki & Abe contour finding core implementation

• Core implementation of Suzuki & Abe (1985) contour finding algorithm with 820 lines
• Defines RetrievalMode enum (External, List, CComp, Tree) and ContourApproximationMode (None,
 Simple)
• Implements trace_border function with 8-directional neighbor scanning and direction lookup tables
• Provides FindContoursExecutor for reusable buffer management across multiple frames
• Includes hierarchy computation with parent-child relationships and border type tracking
• Implements SWAR optimization using u64 word reads to skip zero/all-ones pixel runs
• Contains 15+ unit tests covering edge cases, retrieval modes, and approximation modes

crates/kornia-imgproc/src/contours.rs


2. crates/kornia-imgproc/benches/bench_contours.rs 🧪 Tests +220/-0

Benchmark suite for contour finding performance

• Adds comprehensive benchmark suite comparing kornia vs OpenCV implementations
• Includes three benchmark groups: filled_square, hollow_square, sparse_noise
• Tests six variants per group: kornia (None/Simple), kornia_exec (None/Simple), opencv
 (None/Simple)
• Provides helper functions for generating test images (filled square, hollow square, random noise)
• Uses criterion framework with throughput measurements for performance analysis

crates/kornia-imgproc/benches/bench_contours.rs


3. crates/kornia-imgproc/src/lib.rs ✨ Enhancement +3/-0

Export contours module

• Exports new contours module in public API

crates/kornia-imgproc/src/lib.rs


View more (1)
4. crates/kornia-imgproc/Cargo.toml ⚙️ Configuration changes +5/-0

Add contours benchmark configuration

• Adds bench_contours benchmark configuration with opencv_bench feature requirement

crates/kornia-imgproc/Cargo.toml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Mar 2, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (1)

Grey Divider


Action required

1. find_countours not implemented 📎 Requirement gap ✓ Correctness
Description
The PR introduces find_contours but does not provide the required find_countours API in the
imgproc crate scope. This fails the checklist requirement and may break callers expecting the
specified function name.
Code

crates/kornia-imgproc/src/contours.rs[R121-131]

+/// Convenience API - allocates fresh buffers each call
+/// For repeated use on a series of images prefer FindContoursExecutor
+pub fn find_contours<A: ImageAllocator>(
+    src: &Image<u8, 1, A>,
+    mode: RetrievalMode,
+    method: ContourApproximationMode,
+) -> Result<ContoursResult, String> {
+    let mut img = Vec::new();
+    let mut arena = Vec::new();
+    find_contours_impl(src, mode, method, &mut img, &mut arena)
+}
Evidence
Compliance ID 1 requires a find_countours implementation in the imgproc crate scope; the added
code exposes pub fn find_contours(...) but no find_countours function/alias is present in the
new module.

Implement find_countours (find contours) in imgproc crate
crates/kornia-imgproc/src/contours.rs[121-131]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance checklist requires an API named `find_countours` in the `imgproc` crate scope, but this PR only adds `find_contours`.
## Issue Context
Downstream code/tests may depend on the exact function name required by the checklist.
## Fix Focus Areas
- crates/kornia-imgproc/src/contours.rs[121-131]
- crates/kornia-imgproc/src/lib.rs[69-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Magic numbers in contours📘 Rule violation ⛯ Reliability
Description
The new contour implementation uses hard-coded numeric literals for algorithm parameters (e.g.,
padding size and parallelization threshold), reducing readability and tunability. These values
should be named consts (or configuration) to meet the checklist requirement.
Code

crates/kornia-imgproc/src/contours.rs[R140-163]

+    let height = src.height();
+    let width = src.width();
+    let padded_w = width + 2;
+    let padded_h = height + 2;
+    let padded_n = padded_h * padded_w;
+
+    // Grow or reuse the working image buffer
+    if img.len() < padded_n {
+        img.resize(padded_n, 0);
+    }
+    let img_slice = &mut img[..padded_n];
+    img_slice.fill(0i16);
+
+    // Binarise: parallel for ≥ 512×512 (amortises rayon overhead), sequential below
+    let src_data = src.as_slice();
+    let interior = &mut img_slice[padded_w..padded_w + height * padded_w];
+    if width * height >= 512 * 512 {
+        interior
+            .par_chunks_mut(padded_w)
+            .enumerate()
+            .for_each(|(r, dst_row)| {
+                let src_row = &src_data[r * width..(r + 1) * width];
+                for (d, &s) in dst_row[1..=width].iter_mut().zip(src_row.iter()) {
+                    *d = (s != 0) as i16;
Evidence
Compliance ID 2 prohibits magic numbers for padding/thresholds/loop bounds; the added code uses
literals like + 2 for padding and 512 * 512 for the parallel threshold directly in the
algorithm.

crates/kornia-imgproc/src/contours.rs[140-163]
crates/kornia-imgproc/src/contours.rs[213-226]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The contour-finding implementation contains magic numbers for padding, thresholds, and scan step sizes.
## Issue Context
Per compliance requirements, algorithm parameters should be self-documenting and consistently tunable via named `const`s or configuration.
## Fix Focus Areas
- crates/kornia-imgproc/src/contours.rs[140-163]
- crates/kornia-imgproc/src/contours.rs[213-226]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. unwrap() used in tests📘 Rule violation ⛯ Reliability
Description
New unit tests call fallible APIs using unwrap()/expect(), which can panic instead of
propagating errors. This violates the requirement to avoid panic-based unchecked assumptions even in
tests that exercise fallible APIs.
Code

crates/kornia-imgproc/src/contours.rs[R562-582]

+    fn make_img(w: usize, h: usize, data: Vec<u8>) -> Image<u8, 1, CpuAllocator> {
+        Image(Tensor3::from_shape_vec([h, w, 1], data, CpuAllocator).expect("tensor"))
+    }
+
+    /// 3×3 filled square: 8 border pixels, None approx keeps all
+    #[test]
+    fn test_simple_square_no_approx() {
+        let img = make_img(
+            5,
+            5,
+            vec![
+                0, 0, 0, 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0,
+            ],
+        );
+        let r = find_contours(
+            &img,
+            RetrievalMode::External,
+            ContourApproximationMode::None,
+        )
+        .unwrap();
+        assert_eq!(r.contours.len(), 1);
Evidence
Compliance ID 3 requires using Result/? instead of unwrap()/expect() for fallible
operations; the added tests use .expect("tensor") and .unwrap() on find_contours(...) results.

crates/kornia-imgproc/src/contours.rs[562-564]
crates/kornia-imgproc/src/contours.rs[576-582]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unit tests introduced in this PR use `unwrap()`/`expect()` on fallible operations, which can panic.
## Issue Context
The compliance checklist requires propagating errors via `Result` and `?` (including in tests that invoke fallible APIs).
## Fix Focus Areas
- crates/kornia-imgproc/src/contours.rs[562-582]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. CComp parent type indexed wrong🐞 Bug ✓ Correctness
Description
In RetrievalMode::CComp, the code checks whether a contour’s parent is a hole via
border_types[parent as usize], but parent is an nbd label while border_types is indexed like
hierarchy (sentinel at index 0, border nbd=2 at index 1). This off-by-one breaks the “outer inside
hole” re-rooting logic and can yield incorrect CComp hierarchy.
Code

crates/kornia-imgproc/src/contours.rs[R531-537]

+            let mut fh: Vec<HierarchyEntry> = hierarchy[1..].to_vec();
+            for i in 0..fh.len() {
+                let parent = fh[i][3];
+                let is_outer_inside_hole = parent > 0
+                    && matches!(border_types[i + 1], BorderType::Outer)
+                    && matches!(border_types[parent as usize], BorderType::Hole);
+                if is_outer_inside_hole {
Evidence
determine_parent returns lnbd/stored parent values which are border labels (nbd/lnbd), not
direct indices into border_types. Since border_types has a sentinel at index 0 and pushes each
contour type in tracing order, looking up a parent border type requires indexing by parent-1, not
parent.

crates/kornia-imgproc/src/contours.rs[438-466]
crates/kornia-imgproc/src/contours.rs[189-192]
crates/kornia-imgproc/src/contours.rs[264-267]
crates/kornia-imgproc/src/contours.rs[531-537]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RetrievalMode::CComp` uses `border_types[parent as usize]` to test whether the parent is a hole, but `parent` is stored as an `nbd` border label while `border_types` is indexed with a sentinel at index 0 (i.e., border label `p` maps to index `p-1`). This off-by-one breaks CComp re-rooting decisions.
## Issue Context
`determine_parent` returns `lnbd`/stored parents as border labels. `border_types` is pushed with a sentinel first, then one entry per traced border.
## Fix Focus Areas
- crates/kornia-imgproc/src/contours.rs[438-466]
- crates/kornia-imgproc/src/contours.rs[189-192]
- crates/kornia-imgproc/src/contours.rs[264-267]
- crates/kornia-imgproc/src/contours.rs[531-537]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

5. SWAR depends on endianness🐞 Bug ➹ Performance
Description
The ALL-ones SWAR fast-path uses a hard-coded u64 constant that explicitly assumes little-endian
layout of i16. On big-endian targets this comparison will never match, silently disabling the
optimization (performance regression / inconsistent behavior across platforms).
Code

crates/kornia-imgproc/src/contours.rs[R75-77]

+// Four i16 values equal to 1 packed into a u64
+// NOTE: assumes little-endian byte order
+const ALL_ONES_I16: u64 = 0x0001_0001_0001_0001;
Evidence
The code comments that the packed constant assumes little-endian. That constant is then used for a
hot-loop comparison to skip runs of interior 1 pixels; on non-little-endian targets, the packed
representation differs so the equality check won’t behave as intended.

crates/kornia-imgproc/src/contours.rs[75-77]
crates/kornia-imgproc/src/contours.rs[274-279]
crates/kornia-3d/src/io/pcd/parser.rs[58-78]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The SWAR all-ones fast-path compares a `u64` read from `i16` pixels against a constant that assumes little-endian layout. On big-endian targets the comparison won’t match, silently disabling the optimization.
## Issue Context
The code already calls out the little-endian assumption in a comment.
## Fix Focus Areas
- crates/kornia-imgproc/src/contours.rs[75-77]
- crates/kornia-imgproc/src/contours.rs[274-279]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new contour-finding feature to kornia-imgproc, implementing the Suzuki & Abe (1985) border-following algorithm and exposing a public find_contours API (plus an executor for reuse), along with benchmarks and unit tests.

Changes:

  • Added contours module export to kornia-imgproc.
  • Implemented contour tracing, approximation modes, and hierarchy reconstruction with multiple retrieval modes.
  • Added Criterion benchmark comparing against OpenCV (behind opencv_bench feature).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
crates/kornia-imgproc/src/lib.rs Exposes the new contours module from the crate root.
crates/kornia-imgproc/src/contours.rs Implements contour finding, hierarchy modes, reusable executor, and unit tests.
crates/kornia-imgproc/benches/bench_contours.rs Adds benchmarks for contours (kornia vs OpenCV, incl. executor variants).
crates/kornia-imgproc/Cargo.toml Registers the new bench_contours benchmark gated by opencv_bench.

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 2, 2026

128×128 — Simple approximation

Scenario kornia - Simple kornia - Exec+Simple OpenCV - Simple kornia vs OpenCV exec vs OpenCV
Filled square 5.58 µs 5.20 µs 6.66 µs 1.19x faster 1.28x faster
Hollow square 6.83 µs 6.37 µs 8.82 µs 1.29x faster 1.38x faster
Sparse noise 388.52 µs 382.00 µs 378.16 µs 1.03x slower 1.01x slower

256×256 — Simple approximation

Scenario kornia - Simple kornia - Exec+Simple OpenCV - Simple kornia vs OpenCV exec vs OpenCV
Filled square 18.14 µs 17.46 µs 14.42 µs 1.26x slower 1.21x slower
Hollow square 19.96 µs 18.43 µs 18.47 µs 1.08x slower 1.00x
Sparse noise 1.64 ms 1.61 ms 1.60 ms 1.03x slower 1.01x slower

512×512 — Simple approximation

Scenario kornia - Simple kornia - Exec+Simple OpenCV - Simple kornia vs OpenCV exec vs OpenCV
Filled square 131.32 µs 121.58 µs 70.31 µs 1.87x slower 1.73x slower
Hollow square 131.09 µs 131.25 µs 92.39 µs 1.42x slower 1.42x slower
Sparse noise 7.88 ms 7.83 ms 6.72 ms 1.17x slower 1.16x slower

1024×1024 — Simple approximation

Scenario kornia - Simple kornia - Exec+Simple OpenCV - Simple kornia vs OpenCV exec vs OpenCV
Filled square 285.91 µs 265.09 µs 214.23 µs 1.33x slower 1.24x slower
Hollow square 296.79 µs 272.33 µs 239.28 µs 1.24x slower 1.14x slower
Sparse noise 13.17 ms 13.09 ms 29.17 ms 2.22x faster 2.23x faster

@dhruvkjain
Copy link
Contributor

also please update the crates/kornia-imgproc/README.md to include the new contours module.

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 5, 2026

also please update the crates/kornia-imgproc/README.md to include the new contours module.

Added

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 5, 2026

find_contours -- Visual Reference


Source Image

Source image Binary (threshold = 128)
apriltags_tag36h11 apriltags_tag36h11_binary

ContourApproximationMode

Controls how contour points are stored after tracing.
Demonstrated on the largest outer contour (2950 pts → 1080 pts).

None -- every border pixel (2950 pts) Simple -- corners only (1080 pts)
apriltags_tag36h11_approx_none apriltags_tag36h11_approx_simple

Simple compresses horizontal, vertical, and diagonal straight runs down to
their two endpoints -- from 2950 points to 1080 points
with no loss of shape information.

// Every border pixel stored
find_contours(&image, RetrievalMode::External, ContourApproximationMode::None)?;

// Direction-change endpoints only
find_contours(&image, RetrievalMode::External, ContourApproximationMode::Simple)?;

RetrievalMode

External -- outermost contours only (867 contours)

Returns only contours with no parent. Holes and nested shapes are ignored.
All hierarchy entries are [-1, -1, -1, -1].

apriltags_tag36h11_mode_external
find_contours(&image, RetrievalMode::External, ContourApproximationMode::None)?;
// contours returned: 867

List -- all contours, hierarchy discarded (1739 contours)

Every contour returned in raster-scan order. Hierarchy discarded -- all entries
are [-1, -1, -1, -1].

apriltags_tag36h11_mode_list
find_contours(&image, RetrievalMode::List, ContourApproximationMode::None)?;
// contours returned: 1739

CComp -- two-level hierarchy (1739 contours)

Outer borders at level 1 (green), their holes at level 2 (cyan).
Outer contours found inside holes are re-rooted to level 1.

apriltags_tag36h11_mode_ccomp
find_contours(&image, RetrievalMode::CComp, ContourApproximationMode::None)?;
// contours returned: 1739
// hierarchy[i][3] == -1  → outer (level 1)
// hierarchy[i][3] >= 0   → hole  (level 2, parent = outer)

Tree -- full hierarchy, depth-coloured (1739 contours)

Full parent/child hierarchy preserved. Colour encodes nesting depth:

Depth Colour Meaning
0 Green Outermost contours
1 Orange First-level holes
2 Blue Nested inside depth-1
3 Yellow Nested inside depth-2
4 Red Nested inside depth-3
apriltags_tag36h11_mode_tree
find_contours(&image, RetrievalMode::Tree, ContourApproximationMode::None)?;
// contours returned: 1739

Hierarchy Entry Format -- [i32; 4]

Every contour has a parallel HierarchyEntry = [i32; 4].
Indices are 0-based positions into the contours array; -1 means absent.

Index Field Meaning
[0] next Next contour at the same nesting level, or -1
[1] prev Previous contour at the same nesting level, or -1
[2] first_child First nested contour inside this one, or -1
[3] parent Enclosing contour index, or -1 for top-level

List and External force all four fields to -1.
CComp limits parent depth to 1 (hole -> outer).
Tree has no depth limit.


API

One-shot (fresh buffers each call)

use kornia::imgproc::contours::{find_contours, RetrievalMode, ContourApproximationMode};

let result = find_contours(
    &image,
    RetrievalMode::Tree,
    ContourApproximationMode::Simple,
)?;

for (i, contour) in result.contours.iter().enumerate() {
    let [next, prev, child, parent] = result.hierarchy[i];
    println!("contour {i}: {} pts  parent={parent}", contour.len());
}

FindContoursExecutor -- reuse buffers across frames

Prefer this for video or batch processing. All five internal scratch buffers
(img, arena, ranges, hierarchy, border_types) are retained between
calls. Only ContoursResult is newly allocated per frame.

use kornia::imgproc::contours::FindContoursExecutor;

let mut exec = FindContoursExecutor::new();

for frame in video_frames {
    let result = exec.find_contours(
        &frame,
        RetrievalMode::External,
        ContourApproximationMode::Simple,
    )?;
    // process result ...
}

@dhruvkjain
Copy link
Contributor

@sidd-27 @cjpurackal please review this pr and merge it
can add an example and update docs format in another pr


/// Minimum image area in pixels above which binarisation is parallelised via Rayon
/// Below this threshold the thread-dispatch overhead outweighs the benefit
const PARALLEL_THRESHOLD: usize = 512 * 512;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you have any benchmark numbers for this choice?

Copy link
Contributor Author

@zeel991 zeel991 Mar 6, 2026

Choose a reason for hiding this comment

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

Yes , these are the results
image

I'll change the threshold to 1080p

@sidd-27
Copy link
Collaborator

sidd-27 commented Mar 7, 2026

Review Summary

Excellent implementation of Suzuki & Abe contour finding! 🚀

Strengths:

  • Complete API: Both convenience ind_contours and reusable FindContoursExecutor
  • Performance optimized: SWAR scanning, benchmark-driven parallelization threshold
  • Comprehensive testing: 8 unit tests covering edge cases + OpenCV benchmarks
  • Good error handling: Proper ContoursError enum with hiserror

⚠️ Minor issues to address:

  1. CComp indexing bug: Line ~531-537 has off-by-one in �order_types[parent as usize] - parent is an nbd label but �order_types has sentinel at index 0
  2. Test error handling: Consider using ? operator instead of unwrap() in tests for consistency

🎯 Ready for merge once the CComp bug is fixed

The benchmarks show solid performance characteristics and the code quality is high. Great work @zeel991!

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 7, 2026

Review Summary

Excellent implementation of Suzuki & Abe contour finding! 🚀

Strengths:

  • Complete API: Both convenience ind_contours and reusable FindContoursExecutor
  • Performance optimized: SWAR scanning, benchmark-driven parallelization threshold
  • Comprehensive testing: 8 unit tests covering edge cases + OpenCV benchmarks
  • Good error handling: Proper ContoursError enum with hiserror

⚠️ Minor issues to address:

  1. CComp indexing bug: Line ~531-537 has off-by-one in �order_types[parent as usize] - parent is an nbd label but �order_types has sentinel at index 0
  2. Test error handling: Consider using ? operator instead of unwrap() in tests for consistency

🎯 Ready for merge once the CComp bug is fixed

The benchmarks show solid performance characteristics and the code quality is high. Great work @zeel991!

Tysm, I have pushed the recommended fixes also added a test to check them for assurance

@sidd-27
Copy link
Collaborator

sidd-27 commented Mar 7, 2026

could you look into the failing test? dosent seem related to the pr but just in case

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 7, 2026

could you look into the failing test? dosent seem related to the pr but just in case

I checked ,its not due to my PR

@sidd-27 sidd-27 requested a review from dhruvkjain March 8, 2026 20:20
Copy link
Contributor

@dhruvkjain dhruvkjain left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@sidd-27 sidd-27 left a comment

Choose a reason for hiding this comment

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

i think adding an example would help, lgtm otherwise

@zeel991
Copy link
Contributor Author

zeel991 commented Mar 10, 2026

i think adding an example would help, lgtm otherwise

should I add it in this PR or raise a separate PR for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants