refactor(ata): move target-gain arithmetic to Rust - #805
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds a Rust/PyO3 ATA gain kernel, validates inputs, loads the secondary extension from Python, and delegates candidate scoring from ATA assembly. Tests cover arithmetic parity, delegation, invalid inputs, and extension registration. ChangesATA Rust gain integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ATAAssembly
participant ata_core_loader
participant _core
participant _ata_core
participant RustGainKernel
ATAAssembly->>ata_core_loader: ata_core()
ata_core_loader->>_core: locate shared library
ata_core_loader->>_ata_core: initialize extension
ATAAssembly->>_ata_core: target_information_gains(matrix, cand, target, accum)
_ata_core->>RustGainKernel: validate inputs and calculate gains
RustGainKernel-->>_ata_core: capped-shortfall gains
_ata_core-->>ATAAssembly: gain array
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/fast-mlsirm-py/src/ata_bindings.rs`:
- Around line 76-78: In the ATA binding flow around information_matrix.shape()
and candidate_indices, validate the matrix dimensions and documented ATA limits
before converting candidates or allocating any output buffer. Enforce a maximum
candidate count on the public input before candidate_values.as_slice() and
Vec::with_capacity, while preserving the existing empty-matrix rejection
behavior.
- Around line 71-74: Update py_target_information_gains to normalize PyO3/NumPy
input extraction failures into PyValueError, including wrong-dtype, non-array,
and non-contiguous arguments, instead of allowing TypeError to escape. Validate
the candidate count against the supported limit before Vec allocation or
candidate conversion. Add coverage for wrong-dtype, non-array, non-contiguous,
and oversized-candidate inputs.
In `@python/fast_mlsirm/ata.py`:
- Line 344: Replace the unconditional ata_core() call in the ATA assembly flow
with the established {numpy,rust,auto} backend resolver, retaining Rust as the
default when available and selecting the numerically identical NumPy reference
implementation when the Rust extension is unavailable or lacks PyInit__ata_core.
Preserve the backend contract and add regression coverage for the
unavailable-extension fallback path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d47546d2-7780-49f2-bf1c-b079d4751713
📒 Files selected for processing (7)
crates/fast-mlsirm-py/src/ata_bindings.rscrates/fast-mlsirm-py/src/entrypoint.rsdocs/changelog.d/805-ata-target-gain-rust.mdpython/fast_mlsirm/_ata_core_loader.pypython/fast_mlsirm/ata.pytests/test_ata.pytests/test_modular_extension_coexistence.py
|
@cwl-noema-review Independent exact-head review requested for Review the bounded ATA Rust-ownership slice only. Verify public target-gain arithmetic is Rust/PyO3-owned, Python retains only validation/orchestration/tie-breaking, missing native capability fails closed rather than selecting NumPy, candidate/matrix dimensions and allocations are bounded before conversion, exact errors do not leak PyO3 |
|
Perform a read-only exact-head review of |
Why
Protected
mainstill computes the result-affecting capped-shortfall gain for target-information ATA in Python/NumPy. PR #804 attempted to accelerate that Python numerical owner with broadcasting, but the repository contract and issue #609 require production item-bank selection-objective arithmetic to be Rust-owned.Intentional RED
Exact fail-first head
c6acf7279f8b2d56c98857d78c753defdbbe5e97adds only a public-boundary regression. It injects a fake compiled ATA core and requiresassemble_to_target()to delegate target-gain arithmetic through that boundary. Current protected-main behavior completes assembly but never calls the injected core, so the expected RED is the ownership assertion rather than setup/import/fixture failure.Minimum GREEN
Advances the Rust-first ATA/item-bank numerical boundary in #609 and preserves the performance concern from closed #804 without deepening the Python numerical kernel.
Summary by CodeRabbit
Performance
Reliability
Compatibility
Tests