mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 08:11:12 +00:00
[AZ-334] C1 KLT/RANSAC strategy — engine-rule simple-baseline VIO
Implement KltRansacStrategy, the ADR-002 engine-rule mandatory simple-baseline VioStrategy for E-C1. Pure-Python facade over OpenCV's cv2.goodFeaturesToTrack / calcOpticalFlowPyrLK / findEssentialMat / recoverPose pipeline — no C++/pybind11 binding by design so a Tier-0 workstation runs the strategy with `pip install opencv-python` and the BUILD_KLT_RANSAC=ON gate alone. Constructor + state machine + FDR transition spine mirror Okvis2Strategy + VinsMonoStrategy so the AZ-331 factory + IT-12 comparative harness treat all three as drop-in substitutable; the duplication is the consolidation target now formally in scope for the next cumulative review (batches 52-54). AC coverage: AC-1..AC-11 + NFR-perf mapped to passing tests (25 tests, 23 pass + 2 tier-2 skipped on dev/CI runners; all 25 pass under GPS_DENIED_TIER=2). Honest-covariance invariant (AC-9) implemented as residual-scatter / (N_inliers - 5) with an inlier- count penalty — no client-side floor or smoother; cov Frobenius grows monotonically across DEGRADED. Camera-agnostic source (AC-11) enforced by CI-grep gate that excludes docstring text. Test-Run Cadence: focused suite tests/unit/c1_vio/ green (95 passed, 6 skipped); config-loader + compose-root suites green; full-suite gate deferred to Step 16 per implement skill. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,177 @@
|
||||
# Batch 54 — Cycle 1 Report
|
||||
|
||||
**Date**: 2026-05-14
|
||||
**Tasks**: AZ-334 (C1 KLT/RANSAC Strategy)
|
||||
**Verdict**: COMPLETE — PASS_WITH_WARNINGS
|
||||
|
||||
## Summary
|
||||
|
||||
Implemented `KltRansacStrategy`, the ADR-002 engine-rule mandatory
|
||||
simple-baseline VIO for C1. Pure-Python facade over OpenCV's
|
||||
`cv2.goodFeaturesToTrack` / `cv2.calcOpticalFlowPyrLK` /
|
||||
`cv2.findEssentialMat` / `cv2.recoverPose` pipeline — no C++/pybind11
|
||||
binding by design so a Tier-0 workstation can run the strategy with
|
||||
`pip install opencv-python` and the AZ-331 factory's
|
||||
`BUILD_KLT_RANSAC=ON` gate. Mirrors the AZ-332 OKVIS2 + AZ-333
|
||||
VINS-Mono facade pattern on the orchestration spine so the AZ-331
|
||||
factory + IT-12 comparative harness treat all three strategies as
|
||||
drop-in substitutable.
|
||||
|
||||
## Files added / modified
|
||||
|
||||
### Added (3)
|
||||
|
||||
- `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` —
|
||||
Python facade, ~770 lines.
|
||||
- `tests/unit/c1_vio/test_klt_ransac_strategy.py` — AC-1..AC-11 +
|
||||
NFR-perf + KltRansacConfig validation tests, ~990 lines, 25 tests
|
||||
(23 pass + 2 tier-2 skipped on dev/CI runners).
|
||||
- `_docs/03_implementation/reviews/batch_54_review.md` — code review
|
||||
report.
|
||||
|
||||
### Modified (4)
|
||||
|
||||
- `src/gps_denied_onboard/components/c1_vio/config.py` — add
|
||||
`KltRansacConfig` dataclass + `klt_ransac` field on `C1VioConfig`;
|
||||
`__all__` updated. (Pre-existing `KNOWN_STRATEGIES` already had
|
||||
`klt_ransac` from AZ-331.)
|
||||
- `src/gps_denied_onboard/components/c1_vio/__init__.py` — export
|
||||
`KltRansacConfig`.
|
||||
- `cpp/klt_ransac/CMakeLists.txt` — replace AZ-263 placeholder with
|
||||
a deliberate "pure-Python; no native target" STATUS message so
|
||||
the build graph stays symmetric with `cpp/okvis2/` and
|
||||
`cpp/vins_mono/` while the `BUILD_KLT_RANSAC=ON` flag only gates
|
||||
the Python module import at the AZ-331 composition-root factory.
|
||||
- `tests/unit/c1_vio/test_protocol_conformance.py` — introduce the
|
||||
`_STRATEGIES_WITHOUT_NATIVE_BINDING` category and route
|
||||
`klt_ransac` through it inside
|
||||
`test_ac5_build_vio_strategy_flag_on_but_module_missing` so the
|
||||
parametrised factory test correctly handles the pure-Python
|
||||
shape (no native binding to fail on; the construction should
|
||||
succeed and return a `VioStrategy` instance instead).
|
||||
|
||||
## AC coverage (AC-1..AC-11 + NFR-perf)
|
||||
|
||||
All 11 ACs + NFR-perf mapped to passing tests (see
|
||||
`batch_54_review.md` Phase 2 table). AC-9 + NFR-perf are
|
||||
tier-2-tagged per the task spec; they skip on macOS dev + GitHub
|
||||
Actions Linux runner with `Tier-2-only test; set GPS_DENIED_TIER=2
|
||||
to run`. AC-3 / AC-6 / AC-9 / AC-10 / AC-11 / NFR-perf monkeypatch
|
||||
`cv2.findEssentialMat` + `cv2.recoverPose` +
|
||||
`RansacFilter.filter_correspondences` to deterministic values so
|
||||
the unit suite exercises the FACADE's state machine without
|
||||
depending on real OpenCV geometry on synthetic correspondences;
|
||||
real-geometry validation lives in C1-IT-12 (Jetson Tier-2 fixture).
|
||||
|
||||
## Test results
|
||||
|
||||
### Focused suite — `tests/unit/c1_vio/`
|
||||
|
||||
```
|
||||
95 passed, 6 skipped in 1.67s
|
||||
```
|
||||
|
||||
The 6 skips are the 2 OKVIS2 tier-2 tests + the 2 VINS-Mono tier-2
|
||||
tests + the 2 new KLT/RANSAC tier-2 tests (`test_ac9_*` and
|
||||
`test_nfr_perf_*`).
|
||||
|
||||
### Adjacent regression — config / compose-root
|
||||
|
||||
```
|
||||
17 passed in 1.96s
|
||||
```
|
||||
|
||||
(`test_az269_config_loader.py`, `test_az270_compose_root.py` — all
|
||||
green; the new `KltRansacConfig` registration did not break the
|
||||
schema-loader or compose-root layered-import guards.)
|
||||
|
||||
### Tier-2 verification
|
||||
|
||||
```
|
||||
GPS_DENIED_TIER=2 pytest tests/unit/c1_vio/test_klt_ransac_strategy.py
|
||||
→ 25 passed in 0.43s
|
||||
```
|
||||
|
||||
All 25 tests pass under the tier-2 gate (including AC-9 honest-
|
||||
covariance monotonicity over 48 synthetic frames + NFR-perf p95
|
||||
record).
|
||||
|
||||
### Full suite
|
||||
|
||||
Deferred per the implement skill's Test-Run Cadence — the full
|
||||
unit-suite gate runs exactly once at Step 16 (end of implementation
|
||||
phase), not per-batch. Focused tests + cumulative review (every K
|
||||
batches) catch cross-batch regressions before then.
|
||||
|
||||
## Architectural decisions
|
||||
|
||||
- **No native binding by design**: KLT/RANSAC is pure Python over
|
||||
OpenCV's Python bindings. The `cpp/klt_ransac/CMakeLists.txt`
|
||||
placeholder is preserved (with an explanatory STATUS message) for
|
||||
build-graph symmetry with `cpp/okvis2/` and `cpp/vins_mono/`; the
|
||||
`BUILD_KLT_RANSAC=ON` flag only gates the Python module import at
|
||||
the AZ-331 composition-root factory.
|
||||
- **Constructor shape matches factory**: `KltRansacStrategy(config,
|
||||
*, fdr_client, clock=None)` mirrors `Okvis2Strategy` +
|
||||
`VinsMonoStrategy` so the AZ-331 factory invokes all three via the
|
||||
same call shape. The task spec's illustrative constructor
|
||||
(with explicit injection of `CameraCalibration` / `ImuPreintegrator`
|
||||
/ `RansacFilter` / `Logger`) was deliberately not adopted because
|
||||
it would diverge from the existing factory contract; the same
|
||||
dependencies are resolved internally instead — `RansacFilter` is
|
||||
static (AZ-282 stateless helper) and `ImuPreintegrator` is
|
||||
constructed lazily on the first `process_frame` call (it needs
|
||||
the per-call `CameraCalibration` which is not available at
|
||||
construction time).
|
||||
- **Honest covariance, AC-9 compliant**: per-frame covariance =
|
||||
`np.eye(6) * (sigma_sq + inlier_penalty) / max(inlier_count - 5, 1)`
|
||||
where `sigma_sq = median_residual_px**2` and `inlier_penalty =
|
||||
threshold_px / max(inlier_count, 1)`. No client-side floor or
|
||||
smoother; the formula grows monotonically as inliers drop or
|
||||
residuals scatter. Tier-2 test walks 48 scripted-inlier frames
|
||||
through the DEGRADED window and verifies monotonicity.
|
||||
- **Camera agnostic, AC-11 enforced**: no `adti20` / `adti26`
|
||||
literals in executable source (docstring mentions are excluded
|
||||
from the CI grep via AST-aware stripping in the test). The
|
||||
per-call `CameraCalibration` argument carries intrinsics; the
|
||||
same code path produces sensible `VioOutput` for two distinct
|
||||
calibrations (different f, cx, cy).
|
||||
- **State machine mirrors OKVIS2 / VINS-Mono**: `INIT` until
|
||||
`warm_start_max_frames` is exhausted, then `TRACKING` if inlier
|
||||
count ≥ `min_features_for_pose`, else `DEGRADED`; sustained
|
||||
pose-recovery failure for `lost_frame_threshold` consecutive
|
||||
frames raises `VioFatalError` with state == `LOST`. Exactly one
|
||||
`vio.health` FDR record per transition (AC-10).
|
||||
- **Error envelope closed**: every OpenCV `cv2.error` is caught at
|
||||
each call site and rewrapped into `VioFatalError` with
|
||||
`__cause__` chaining (AC-4). `RansacFilterError` +
|
||||
`ImuPreintegrationError` are also caught and rewrapped.
|
||||
|
||||
## Out of scope / deferred
|
||||
|
||||
- Real geometry validation against Derkachi fixtures — Tier-2
|
||||
follow-up; C1-IT-12 binds KLT/RANSAC alongside OKVIS2.
|
||||
- Warm-start hint persistence (AZ-335) — separate batch.
|
||||
- Strategy-facade consolidation hygiene PBI — now formally in scope
|
||||
for the cumulative review covering batches 52-54 (next trigger).
|
||||
All three strategy shapes (OKVIS2, VINS-Mono, KLT/RANSAC) are
|
||||
now visible so the right factoring (template-method base class
|
||||
for the orchestration spine + per-strategy geometry hook) can be
|
||||
scoped without over-fitting.
|
||||
|
||||
## Honest-covariance numeric envelope (AC-9 evidence)
|
||||
|
||||
Tier-2 test walks the following inlier-count sequence through the
|
||||
DEGRADED gate (`min_features_for_pose=50`):
|
||||
|
||||
| Frame range | Inlier count | State | Cov Frobenius (approx) |
|
||||
|-------------|--------------|----------|------------------------|
|
||||
| 1 | first-frame | INIT | 24.49 (10·√6) |
|
||||
| 2..29 | 80 | TRACKING | ≈ 0.0056 |
|
||||
| 30 | 40 | DEGRADED | ≈ 0.013 |
|
||||
| 31..43 | 35..12 | DEGRADED | strictly increasing |
|
||||
| 44..49 | 10 | DEGRADED | ≈ 0.127 |
|
||||
|
||||
The Frobenius norm grows monotonically across the entire DEGRADED
|
||||
window — the AC-9 honest-covariance invariant is upheld by the
|
||||
formula's structure, not by a floor clamp.
|
||||
@@ -0,0 +1,242 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 54 (AZ-334 — C1 KLT/RANSAC Strategy)
|
||||
**Date**: 2026-05-14
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Scope
|
||||
|
||||
Single-task batch implementing `KltRansacStrategy`, the mandatory
|
||||
simple-baseline `VioStrategy` that satisfies the ADR-002 engine rule
|
||||
(every component MUST ship a simple-baseline strategy alongside its
|
||||
production-default). Pure-Python over OpenCV's `cv2.goodFeaturesToTrack`
|
||||
/ `cv2.calcOpticalFlowPyrLK` / `cv2.findEssentialMat` / `cv2.recoverPose`
|
||||
path; no C++/pybind11 native binding by design — Tier-0 workstation can
|
||||
run the strategy with `pip install opencv-python` only.
|
||||
|
||||
### Changed files
|
||||
|
||||
- `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` — new
|
||||
Python facade (~770 lines, including module docstring + AC mapping
|
||||
+ risk-mitigation notes).
|
||||
- `src/gps_denied_onboard/components/c1_vio/config.py` — add
|
||||
`KltRansacConfig` dataclass + `klt_ransac` field on `C1VioConfig`;
|
||||
`__all__` updated; `KNOWN_STRATEGIES` already had `klt_ransac`.
|
||||
- `src/gps_denied_onboard/components/c1_vio/__init__.py` — export
|
||||
`KltRansacConfig`.
|
||||
- `cpp/klt_ransac/CMakeLists.txt` — replace placeholder message
|
||||
with a deliberate "pure-Python; no native target" explanation so
|
||||
the build graph stays symmetric with `cpp/okvis2/` and
|
||||
`cpp/vins_mono/` while the `BUILD_KLT_RANSAC=ON` flag only gates
|
||||
the Python module import at the AZ-331 composition-root factory.
|
||||
- `tests/unit/c1_vio/test_klt_ransac_strategy.py` — new test module
|
||||
covering AC-1..AC-11 + NFR-perf (~990 lines, 25 tests; 23 pass + 2
|
||||
tier-2 skipped on dev/CI runners).
|
||||
- `tests/unit/c1_vio/test_protocol_conformance.py` — introduce the
|
||||
`_STRATEGIES_WITHOUT_NATIVE_BINDING` category and route
|
||||
`klt_ransac` through it inside `test_ac5_build_vio_strategy_flag_on_but_module_missing`
|
||||
so the parametrised factory test correctly handles the pure-Python
|
||||
shape (no native binding to fail on).
|
||||
|
||||
## Phase 2 — Spec Compliance
|
||||
|
||||
| AC | Test | Verified |
|
||||
|-------|----------------------------------------------------------------------------|----------|
|
||||
| AC-1 | `test_ac1_current_strategy_label_returns_klt_ransac` + | ✓ |
|
||||
| | `test_ac1_constructor_rejects_mismatched_strategy_label` | |
|
||||
| AC-2 | `test_ac2_first_frame_emits_init_state_with_identity_pose` | ✓ |
|
||||
| AC-3 | `test_ac3_steady_state_frame_emits_pose_and_spd_covariance` | ✓ |
|
||||
| AC-4 | `test_ac4_cv2_error_in_find_essential_mat_rewrapped_to_vio_fatal_error` + | ✓ |
|
||||
| | `test_ac4_cv2_error_in_recover_pose_rewrapped_to_vio_fatal_error` | |
|
||||
| AC-5 | `test_ac5_reset_to_warm_start_clears_feature_buffer_and_seeds_bias` + | ✓ |
|
||||
| | `test_ac5_reset_to_warm_start_idempotent_across_consecutive_calls` + | |
|
||||
| | `test_ac5_reset_to_warm_start_rejects_non_pose3_hint` | |
|
||||
| AC-6 | `test_ac6_low_inlier_count_emits_degraded_with_monotonic_covariance` | ✓ |
|
||||
| AC-7 | `test_ac7_sustained_pose_recovery_failure_raises_vio_fatal_error` | ✓ |
|
||||
| AC-8 | `test_ac8_strategy_module_not_imported_at_package_load` + | ✓ |
|
||||
| | `test_protocol_conformance.py::test_ac5_build_vio_strategy_flag_*` | |
|
||||
| AC-9 | `test_ac9_honest_covariance_monotonic_during_degraded` (tier2) | ✓ |
|
||||
| AC-10 | `test_ac10_fdr_vio_health_emitted_per_transition` | ✓ |
|
||||
| AC-11 | `test_ac11_source_has_no_camera_id_literals` + | ✓ |
|
||||
| | `test_ac11_strategy_handles_two_distinct_calibrations` | |
|
||||
| NFR-perf | `test_nfr_perf_process_frame_records_p95` (tier2) | ✓ |
|
||||
|
||||
All 11 ACs + NFR-perf mapped to passing tests. Test suite reports
|
||||
25 tests, 23 pass + 2 tier2 skipped on the standard dev/CI runner;
|
||||
all 25 pass under `GPS_DENIED_TIER=2`. Adjacent regression:
|
||||
`tests/unit/c1_vio/` reports 95 passed + 6 skipped (6 = the 2
|
||||
KLT-tier2 + 2 OKVIS2-tier2 + 2 VINS-Mono-tier2 tests); config-loader
|
||||
and compose-root suites green (17 passed).
|
||||
|
||||
## Phase 3 — Code Quality
|
||||
|
||||
- **SOLID**: `KltRansacStrategy` has a single responsibility (Python
|
||||
facade over OpenCV's KLT/RANSAC path). Constructor injection per
|
||||
ADR-009 — `Config` + `FdrClient` enter explicitly; `Clock` is
|
||||
optional with a `WallClock` default; the AZ-276 `ImuPreintegrator`
|
||||
is constructed lazily on the first `process_frame` call (it
|
||||
requires the per-call `CameraCalibration` which is not available
|
||||
at constructor time — matches the existing factory pattern across
|
||||
OKVIS2 / VINS-Mono).
|
||||
- **Error handling**: error envelope closed at the `VioError` family;
|
||||
every OpenCV `cv2.error` is caught at each call site and rewrapped
|
||||
into `VioFatalError` with `__cause__` chaining (AC-4). Pose-recovery
|
||||
failures route through `_pose_recovery_failed` which raises
|
||||
`VioInitializingError` until `lost_frame_threshold` is exhausted,
|
||||
then escalates to `VioFatalError` (AC-7). RansacFilterError +
|
||||
ImuPreintegrationError are also caught and rewrapped.
|
||||
- **Naming**: matches the OKVIS2 / VINS-Mono facade naming exactly
|
||||
(intentional — IT-12 harness substitutability).
|
||||
- **Complexity**: `process_frame` is ~120 lines; the dominant cost is
|
||||
the explicit step-numbered ladder (IMU push → grayscale → first-frame
|
||||
branch → KLT track → RANSAC filter → essential-matrix → pose recover
|
||||
→ covariance → VioOutput build → state classify → re-seed features).
|
||||
Splitting further would obscure the linear flow that maps 1:1 onto
|
||||
the task spec's "Outcome" numbered list.
|
||||
- **DRY**: structural duplication with OKVIS2 / VINS-Mono facades —
|
||||
see F1 below; deliberately deferred to the post-batch-54 hygiene
|
||||
PBI (now scheduled by the next cumulative review, batches 52-54).
|
||||
- **Test quality**: each AC has a behaviourally-meaningful assertion
|
||||
(covariance SPD, frame_id echoed, transition states ordered,
|
||||
monotonic covariance growth during DEGRADED, etc.). No "did not
|
||||
throw" placeholder tests. AC-3 / AC-6 / AC-9 / AC-10 / AC-11 /
|
||||
NFR-perf monkeypatch `cv2.findEssentialMat` + `cv2.recoverPose`
|
||||
+ `RansacFilter.filter_correspondences` to deterministic values so
|
||||
the unit suite exercises the FACADE's state machine without
|
||||
depending on real OpenCV geometry on synthetic correspondences;
|
||||
real-geometry validation lives in C1-IT-12 (Jetson Tier-2 fixture).
|
||||
- **Dead code**: none. `_drain_into_list` removed from the test
|
||||
helper after the simpler `_drain` was introduced.
|
||||
|
||||
## Phase 4 — Security Quick-Scan
|
||||
|
||||
- No SQL / command injection paths. No `subprocess(shell=True)`,
|
||||
`eval`, `exec`.
|
||||
- No hardcoded secrets, API keys, or credentials.
|
||||
- Input validation: `_intrinsics_3x3` rejects non-3x3 K with
|
||||
`VioFatalError`; `_grayscale` rejects unsupported image shapes;
|
||||
`KltRansacConfig.__post_init__` validates every knob range
|
||||
(max_corners ≥ 4, klt_window_size_px odd ≥ 3, klt_pyramid_levels
|
||||
≥ 1, min_features_for_pose ≥ 5, ransac_inlier_ratio in (0, 1],
|
||||
essential_matrix_ransac_threshold_px > 0).
|
||||
- Sensitive data: per-frame DEBUG log defaults OFF
|
||||
(`KltRansacConfig.per_frame_debug_log = False`) — matches
|
||||
`description.md` § 9 logging hygiene.
|
||||
|
||||
PASS.
|
||||
|
||||
## Phase 5 — Performance Scan
|
||||
|
||||
- Hot path: `process_frame`. IMU push loop is
|
||||
`O(samples_per_window)` — unavoidable. KLT track + RANSAC are
|
||||
multi-threaded internally by OpenCV; bound at 30 % of one core
|
||||
per ADR-002 budget partition.
|
||||
- No N+1, no unbounded buffers (FdrClient capacity bounded by
|
||||
AZ-273 ringbuf; the strategy keeps a single
|
||||
`_prev_features` numpy array sized at `max_corners`).
|
||||
- Covariance estimator: `_estimate_covariance` does one
|
||||
`np.eye(6) * scalar` — O(1).
|
||||
- AC-9 honest-covariance: the residual_var / DOF formula has no
|
||||
client-side floor; cov Frobenius grows monotonically as
|
||||
inlier_count drops (verified in tier-2 test).
|
||||
|
||||
PASS.
|
||||
|
||||
## Phase 6 — Cross-Task Consistency
|
||||
|
||||
N/A — single-task batch. Cross-task consistency with AZ-332 +
|
||||
AZ-333 is tracked in the next cumulative review (batches 52-54),
|
||||
which will see all three strategy facades and the duplication
|
||||
finding (F1) at the same time.
|
||||
|
||||
## Phase 7 — Architecture Compliance
|
||||
|
||||
- **Layer direction**: `klt_ransac.py` imports from `_types.nav`,
|
||||
`_types.calibration` (TYPE_CHECKING only), `clock.wall_clock`,
|
||||
`components.c1_vio.config` (TYPE_CHECKING only),
|
||||
`components.c1_vio.errors`, `fdr_client`, `helpers.imu_preintegrator`,
|
||||
`helpers.ransac_filter`, `logging` — all L1/L2 substrate per the
|
||||
c1 layering. PASS.
|
||||
- **Public API respect**: `KltRansacConfig` exported through
|
||||
`c1_vio/__init__.py`; `KltRansacStrategy` deliberately NOT exported
|
||||
(lazy import only via `runtime_root.vio_factory`) — matches the
|
||||
Risk-2/Risk-3 pattern from OKVIS2 and VINS-Mono. PASS.
|
||||
- **No new cyclic dependencies**: introduced module is a leaf — no
|
||||
back-edges to its own importers.
|
||||
- **Native binding location**: NONE by design. `cpp/klt_ransac/`
|
||||
carries a CMakeLists that returns a STATUS message documenting
|
||||
the absence of native target; the directory is preserved for
|
||||
build-graph symmetry with `cpp/okvis2/` and `cpp/vins_mono/`.
|
||||
- **Build flag respect**: `BUILD_KLT_RANSAC=OFF` keeps the
|
||||
composition-root factory from importing the strategy module; the
|
||||
AZ-331 factory raises `StrategyNotAvailableError` before any
|
||||
import — Risk-3 mitigation intact for operator-tooling binaries
|
||||
(which do not need any VIO at all).
|
||||
- **AC-11 camera agnostic**: source CI-grep gate verifies no
|
||||
`adti20` / `adti26` literals in executable code (docstrings are
|
||||
excluded via AST-aware stripping in the test). PASS.
|
||||
|
||||
PASS.
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Low | Maintainability | `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` | Structural duplication with `okvis2.py` / `vins_mono.py` — now scheduled for post-batch-54 hygiene PBI via cumulative review |
|
||||
| 2 | Low | Maintainability | `tests/unit/c1_vio/test_klt_ransac_strategy.py` | `_patch_pose_recovery` helper is bespoke per-strategy; the same patching pattern could plausibly be shared with OKVIS2 / VINS-Mono fake-binding fixtures |
|
||||
|
||||
### Finding details
|
||||
|
||||
**F1: Structural duplication of strategy facade** (Low / Maintainability)
|
||||
|
||||
- Location: `src/gps_denied_onboard/components/c1_vio/klt_ransac.py`
|
||||
vs `src/gps_denied_onboard/components/c1_vio/okvis2.py` /
|
||||
`vins_mono.py`
|
||||
- Description: The new `KltRansacStrategy` mirrors `Okvis2Strategy`
|
||||
+ `VinsMonoStrategy` ~70 % verbatim on the orchestration spine —
|
||||
`_classify_state`, `_tick_lost`, `_emit_transition`,
|
||||
`_bias_norm`, `_now_iso`, `_se3_from_4x4`, the constructor strategy-
|
||||
label guard, and the FDR record-emit shape are byte-equivalent
|
||||
modulo strategy-label constants. The geometry-specific pipeline
|
||||
(KLT seed/track, RANSAC filter, findEssentialMat, recoverPose,
|
||||
residual-scatter covariance) IS unique to this strategy and lives
|
||||
in its own module — that boundary is correct. The shared
|
||||
orchestration spine is the consolidation target tracked by the
|
||||
hygiene PBI deferred since batch 53; the cumulative review
|
||||
scheduled for batches 52-54 (next trigger) will formally raise the
|
||||
PBI now that all three strategy shapes are visible.
|
||||
- Suggestion: defer (one more time) to the cumulative review for
|
||||
batches 52-54 — the right factoring is now visible (template-
|
||||
method base class for the orchestration spine + per-strategy
|
||||
geometry hook). Do NOT create the PBI ad-hoc here; let the
|
||||
cumulative review own the cross-batch refactor scope.
|
||||
- Task: AZ-334
|
||||
|
||||
**F2: Test patching helper could be shared** (Low / Maintainability)
|
||||
|
||||
- Location: `tests/unit/c1_vio/test_klt_ransac_strategy.py`
|
||||
(`_patch_pose_recovery`) vs `tests/unit/c1_vio/conftest.py`
|
||||
(`FakeOkvis2Backend` / `FakeVinsMonoBackend`)
|
||||
- Description: KLT/RANSAC uses real OpenCV bindings (no fake-binding
|
||||
fixture) so the existing conftest fakes don't apply directly. The
|
||||
per-test helper `_patch_pose_recovery` does the equivalent job of
|
||||
forcing a deterministic-success path. This is a different
|
||||
abstraction shape from the conftest fakes but lives at the same
|
||||
layer; consolidating with the post-batch-54 hygiene PBI would let
|
||||
all three strategies share a single per-strategy "ScriptedSuccess"
|
||||
fixture surface.
|
||||
- Suggestion: same as F1 — let the cumulative review's hygiene PBI
|
||||
own the cross-cutting test-fixture refactor.
|
||||
- Task: AZ-334
|
||||
|
||||
## Verdict
|
||||
|
||||
**PASS_WITH_WARNINGS** — two Low-severity duplication findings, both
|
||||
intentionally deferred and now formally in scope for the next
|
||||
cumulative review (batches 52-54). No Critical, High, or Medium
|
||||
findings. All 11 ACs + NFR-perf covered with passing tests.
|
||||
|
||||
Pre-existing environment-dependent perf flake noted in batch 53
|
||||
(`tests/unit/c12_operator_orchestrator/test_cli_console_script.py::test_cold_start_under_500ms_p99`)
|
||||
is still environmental and untouched by this batch — reported, not
|
||||
blocking.
|
||||
@@ -12,5 +12,6 @@ sub_step:
|
||||
retry_count: 0
|
||||
cycle: 1
|
||||
tracker: jira
|
||||
last_completed_batch: 53
|
||||
last_completed_batch: 54
|
||||
last_cumulative_review: batches_49-51
|
||||
current_batch: 55
|
||||
|
||||
Reference in New Issue
Block a user