[codex] fix python id coercion review follow-ups

[Fieldnote-Echo]

Summary

Follow up on outstanding PR #90 bot review findings after #90/#91 were merged:

• clarify the module-level FFI boundary docs so candidate/doc-id arrays are called out as the intentional non-contiguous exception
• handle non-contiguous uint32 candidate/doc-id arrays directly in the as_u32_ids_1d fast path by copying them, instead of falling through to the generic checked conversion macro
• remove the now-redundant try_int_dtype!(u32) path and simplify the dtype-order comment

Why

PR #90 changed candidate/doc-id inputs from strict uint32 to checked integer coercion. Two bot findings were still valid after the combined merge:

• Qodo noted the module docs still implied all non-C-contiguous arrays are rejected.
• Gemini noted the uint32 helper path could handle both contiguous and non-contiguous arrays without a redundant cast/conversion pass.

The Qodo range-string spacing finding was investigated separately and not changed: Rust string continuations do not preserve the newline indentation in the emitted message.

Validation

• cargo fmt -p ordvec-python --check
• cargo clippy -p ordvec-python --all-targets -- -D warnings
• maturin develop
• .venv/bin/pytest tests/test_input_guards.py

[gemini-code-assist]

Code Review

This pull request optimizes the handling of non-contiguous uint32 arrays in as_u32_ids_1d by copying them directly within the fast-path check, avoiding the fallback macro path. Feedback suggests using .to_vec() on the ndarray view instead of manually iterating and collecting, which is more idiomatic and efficient.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Review Summary by Qodo

Handle non-contiguous uint32 arrays in id coercion fast path

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Clarify FFI boundary docs: candidate/doc-id arrays support non-contiguous layouts
• Handle non-contiguous uint32 arrays directly in fast path via copying
• Remove redundant try_int_dtype!(u32) macro invocation and simplify comments

Diagram

flowchart LR
  A["Non-contiguous uint32 array"] -->|"Previously fell through"| B["Generic checked conversion"]
  A -->|"Now handled directly"| C["Copy via iterator"]
  C --> D["CandidateIds::Owned"]
  E["Contiguous uint32 array"] -->|"Zero-copy borrow"| F["CandidateIds::Borrowed"]
  G["Other integer dtypes"] -->|"Checked conversion"| D

Loading

File Changes

1. ordvec-python/src/lib.rs 🐞 Bug fix +10/-7

Optimize uint32 id coercion and clarify FFI docs

• Updated module-level FFI boundary documentation to clarify that candidate and doc-id arrays
 support non-contiguous layouts through copying, unlike other array inputs
• Enhanced as_u32_ids_1d fast path to directly handle non-contiguous uint32 arrays by copying
 them without redundant bounds checks
• Removed the now-unnecessary try_int_dtype!(u32) macro invocation that was previously used to
 handle non-contiguous uint32 arrays
• Simplified dtype-order comment to reflect that order is irrelevant since each downcast performs
 exact dtype matching

ordvec-python/src/lib.rs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Docs misstate u32 checks ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Module-level FFI docs say non-contiguous candidate/doc-id arrays are copied through the checked
u32 conversion path, but non-contiguous uint32 arrays are copied directly without the checked
try_from loop. This can mislead maintainers about where bounds checks occur and which paths
allocate/copy.

Code

ordvec-python/src/lib.rs[R18-21]

Evidence

The module docs claim non-contiguous integer arrays are copied via the checked u32 conversion
path, but the uint32 non-contiguous branch copies directly without u32::try_from. The checked
loop is only applied in the try_int_dtype! macro for non-u32 integer dtypes.

ordvec-python/src/lib.rs[14-21]
ordvec-python/src/lib.rs[142-153]
ordvec-python/src/lib.rs[155-180]

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 module-level docs state that non-contiguous candidate/doc-id integer arrays are copied via the *checked* `u32` conversion path, but the implementation now copies non-contiguous `uint32` arrays directly (no `u32::try_from` bounds-check loop). This makes the docs inaccurate and can confuse future maintenance/debugging.

### Issue Context
- The checked conversion loop (`u32::try_from`) is only used for non-`u32` integer dtypes.
- Non-contiguous `uint32` arrays are copied with `iter().copied().collect()`.

### Fix Focus Areas
- ordvec-python/src/lib.rs[14-21]
- ordvec-python/src/lib.rs[125-141]

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

---

[Copilot]

Pull request overview

Follow-up to prior Python FFI boundary work for candidate/doc-id inputs: aligns the crate-level docs with the intended “non-contiguous IDs are allowed (via copy)” exception, and optimizes the uint32 fast-path to avoid redundant checked conversions for non-contiguous uint32 arrays.

Changes:

• Clarify module-level FFI boundary documentation to call out candidate/doc-id non-contiguous handling as an exception.
• Improve as_u32_ids_1d fast-path to directly copy non-contiguous uint32 inputs (while still zero-copy borrowing contiguous uint32).
• Simplify the dtype matching order/comments by removing the redundant try_int_dtype!(u32) path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Fieldnote-Echo]

Bot review disposition for top-level findings:

• Qodo docs-misstate-u32 checks: valid, low risk / small effort. Fixed in 70d579a by updating the module-level FFI boundary docs to reflect the exact paths for contiguous uint32, non-contiguous uint32, and other integer dtypes.
• Gemini to_vec() suggestion: valid, low risk / small effort. Also fixed in 70d579a.

Validation:

• cargo fmt --all --check
• cargo clippy --all-targets --all-features -- -D warnings