Repository navigation
Commit 1d1f9a3
Reduce max function complexity in opensfm and seamseg
Summary:
Brings the RLAIR P0 `high_cyclomatic_complexity_max_function` files under the target (every function CCN < 20, measured with `lizard` 1.24.1):
| File | Before (function) | After (max function) |
|---|---|---|
| `opensfm/src/bundle/src/bundle_adjuster.cc` | 74 (`BundleAdjuster::Run`) | 14 (`ComputeCovariances`, unchanged) |
| `opensfm/src/sfm/src/map_helpers.cc` | 26 (`FilterBadlyConditionedPoints`) | 10 (`RemoveIsolatedPoints`, unchanged) |
| `opensfm/src/sfm/src/ba_helpers.cc` | 24 (`Bundle`), also 21 `BundleLocal`, 20 `BundleShotPoses` | 14 (`AlignmentConstraints`, unchanged) |
| `annotation_gui_gcp/run_ba.py` | 41 (`align`) | 9 (`align_3d_annotations_to_reconstruction`, unchanged) |
| `seamseg/src/bbx/cabb_loss.cpp` | 25 (`solve_1d`) | 8 (`cabb_loss_solver_impl`, unchanged) |
Approach:
- `BundleAdjuster::Run`: split into one private helper per block type (parameter blocks, reconstruction scales, point priors, scale groups / std-deviation blocks, rig instance priors, rig camera priors, camera priors, reprojection, relative motion / rotation, common position, heatmaps, up vector, pan/tilt/roll, linear motion, gauge fix), called in the original order. Shared pieces are file-local helpers: `AddParameterBlockWithLock`, `AddDualTransitionBarrier`, `TwoShotsParameterBlocks` (relative rotation + common position), `LinearMotionParameterBlocks`, and one `AddAbsoluteAngleErrors<Error>` template for pan/tilt/roll. Residual and parameter block insertion order, loss-function creation and ceres options are unchanged; only new private helpers in the header.
- `map_helpers.cc`: `FilterBadlyConditionedPoints` now uses `CollectLandmarkViews`, `HasWideEnoughRays`, `ComputeConditionNumber` (returns `std::optional`), `AppendConditionOutliers`, `RemoveLandmarks`. Removal order is unchanged.
- `ba_helpers.cc`: file-local helpers for the code the three bundle entry points shared (loss/prior config, wall times, GPS averaging `GpsAverage`, fixed rig cameras) and per-function rig-instance / observation helpers. No header change.
- `cabb_loss.cpp`: `solve_1d`'s nine section searches go through one `Solve1dState::search` helper; the L1/L2 section intervals are built by `center_loss_sections` / `width_loss_sections`. All formulas are the original expressions.
- `run_ba.py`: `align` split into `_has_input_files`, `_rigidly_align_pair`, `_resect_into_reconstruction`, `_log_gcp_errors`, `_bundle_merged_reconstruction`, `_save_gcp_reprojections`, `_log_summary`. The metrics file keeps all keys (shot-std keys stay -1, as for every non-`--covariance` run before).
`run_ba.py` `--covariance`: removed. `bundle_with_fixed_images` called `BundleAdjuster.add_shot` / `get_shot`, which no longer exist, so `--covariance` always crashed. `pybundle` has no per-shot / per-rig-instance covariance getter (only `get_covariance_estimation_valid`), so there was nothing to port it to without a new C++ binding. Removed `bundle_with_fixed_images`, `decompose_covariance`, the `--covariance` flag and the `--std-threshold` flag (only used to classify the covariance results). The GCP annotation GUI (`lib/GUI.py`) passed `--covariance` from its default "full" analysis, so that button crashed too; it no longer passes the flag.
Bugs found, fixed minimally:
1. `BAHelpers::BundleShotPoses` called `AddRigInstance` and `AddRigInstancePositionPrior` inside the per-shot loop, dividing the running GPS sum by the count on every iteration. For a moving rig instance with 3+ GPS shots the prior was wrong (e.g. `((p1+p2)/2 + p3)/3`). They now run once per instance after the loop, as in `BundleLocal`. Added `test_bundle_shot_poses_rig_gps_average`, which fails without the fix.
2. `cabb_loss.cpp` `solve_2d` passed uninitialized `dc_con1/dw_con1/dc_con2/dw_con2` to `solve_1d`, which reads `dw` for its starting loss (undefined behaviour). They are now zero-initialized, the same as the `zeros_like` output buffers on the direct `solve_1d` path. Because of the UB, any code change would perturb this path: refactored code without the init gave different outputs on every run. With the init, the 1D paths are bit-identical to before. Only rows on the 2D path change (0.04-3.6% of rows in the check below), and the new solution has lower loss in >98% of them. Follow-up, not changed here: `solve_1d` seeds the minimum with `loss(..., dw)`, where `min_dw` looks intended. With `dw == 0`, the boundary candidate is never chosen.
Reviewed By: millenbop
Differential Revision: D125282253
fbshipit-source-id: 5474ae3dced239d88a9d47c29ccbda6ff2a30bf91 parent 39c58a8 commit 1d1f9a3
7 files changed
Lines changed: 1055 additions & 1097 deletions
File tree
- annotation_gui_gcp
- lib
- opensfm
- src
- bundle
- src
- sfm/src
- test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
100 | 100 | | |
101 | 101 | | |
102 | 102 | | |
103 | | - | |
| 103 | + | |
104 | 104 | | |
105 | 105 | | |
106 | | - | |
| 106 | + | |
107 | 107 | | |
108 | | - | |
| 108 | + | |
109 | 109 | | |
110 | 110 | | |
111 | 111 | | |
| |||
132 | 132 | | |
133 | 133 | | |
134 | 134 | | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | 135 | | |
139 | 136 | | |
140 | 137 | | |
| |||
0 commit comments