fix(patchwork): close the 2.3 F1 gap vs the original Patchwork (+ USAGE.md)#90
Merged
Conversation
…ile flags)
cpp/patchwork/src/patchwork.cpp deviates from the original
~/git/patchwork in three places that together cost about 2.3 F1 on
KITTI 00-10 under the Patchwork++ paper evaluation protocol with
paper-matched parameters (uprightness_thr=0.707, using_global_thr=false).
The fixes are gated behind PW_FIX_1, PW_FIX_2, PW_FIX_3 compile flags so
an ablation can measure each independently:
PW_FIX_1 — elevation_thr is GROUND-frame; subtract sensor_height to
get the sensor-frame cutoff (the YAML in ~/git/patchwork documents
this explicitly; the reimpl was using the raw value).
PW_FIX_2 — plane-distance comparison: original uses uncentred
`normal . p` against `th_dist - d_`. The reimpl uses the centred
`normal . (p - mean)` against the same threshold, which shifts the
cutoff by an extra `-2 * d_` (about 3.2 m on KITTI ground).
PW_FIX_3 — tier index for elevation/flatness uses the GLOBAL ring
index across all zones, not the local ring-or-zone-id collapse.
Full sweep on KITTI 00-10 (23,201 frames), Patchwork++ paper protocol,
paper params:
current main : P=89.70 R=98.49 F1=93.73
fix/performance : P=94.64 R=97.58 F1=96.02
~/git/patchwork : P=94.38 R=97.90 F1=96.05 (reference)
paper Table I : P=94.23 R=97.62 F1=95.88
Build a variant with:
pip install --no-build-isolation --force-reinstall \\
--config-settings=cmake.define.PW_FIX_1=ON \\
--config-settings=cmake.define.PW_FIX_2=ON \\
--config-settings=cmake.define.PW_FIX_3=ON \\
./python/
See #89 for the full ablation report.
Also adds:
- python/examples/evaluate_semantickitti.py — KITTI eval driver with
both Patchwork-paper and Patchwork++ paper protocols (#87, #88).
- python/examples/aggregate_original_patchwork.py — aggregates the
per-sequence txt files from ~/git/patchwork's eval mode.
- scripts/ablation_sweep.sh — the 5-variant ablation runner.
…+ USAGE.md
The previous commit on this branch gated the three fixes behind compile
flags (PW_FIX_1/2/3) for ablation. Measurement showed all three are
needed to match the original Patchwork; this commit removes the toggles
and applies the fixes unconditionally:
1. tier index = global ring index across all zones (not the previous
`(zone==0) ? ring : zone` collapse).
2. elevation_thr is converted to the sensor frame by subtracting
sensor_height (matches the comment in the original
config/velodyne64.yaml).
3. plane-distance comparison uses uncentred `normal . p` so the cutoff
of `th_dist - d_` actually represents "signed distance < th_dist"
(the previous centred form shifted the cutoff by an extra -d_).
The ablation runner (scripts/ablation_sweep.sh) is removed since it
referenced the compile flags.
Adds USAGE.md covering:
- the two SemanticKITTI evaluation protocols (Patchwork vs Patchwork++
paper) and which to pick when reproducing each paper's Table I,
- parameter tuning order (sensor_height -> uprightness_thr -> range
bounds -> plane-fit thresholds -> elevation_thr / RNR / RVPF / TGR),
- a copy-pasteable command to reproduce Patchwork++ Table I.
See #87, #88, #89.
….patchwork Bumps: - python/pyproject.toml 1.2.0 -> 1.3.0 - cpp/CMakeLists.txt 1.2.0 -> 1.3.0 Adds CHANGELOG.md with a v1.3.0 entry documenting the three performance fixes in cpp/patchwork/src/patchwork.cpp (full sweep: F1 93.73 -> 96.02 under the Patchwork++ paper evaluation protocol; matches the original Patchwork ROS 2 build and paper Table I within paper variance). pypatchworkpp.patchworkpp is unaffected. See #87, #88, #89, #90.
This was referenced May 21, 2026
LimHyungTae
added a commit
that referenced
this pull request
May 21, 2026
…th (#94) cpp/patchwork/ and cpp/patchworkpp/ each carried their own copies of PointXYZ, PCAFeature, and the small geometry helpers (xy2theta, xy2radius, point_z_cmp). The plane-fit SVD itself lived in three slightly-different forms across cpp/patchwork/src/patchwork.cpp, cpp/patchworkpp/src/patchworkpp.cpp, and the upstream ~/git/patchwork. PR #90 (Fix 2) was a concrete bug caused by that drift. This commit factors them into a new tiny static library: cpp/common/ include/patchwork/types.h — PointXYZ, PCAFeature, PatchStatus include/patchwork/plane_fit.h — xy2theta, xy2radius, point_z_cmp, estimate_plane(seeds, out, th_dist) src/plane_fit.cpp — SVD-based plane fit (single canonical implementation) PCAFeature now carries the `principal_` (largest singular vector) field that exists in the original Patchwork repo; the classic Patchwork's PCAFeature was missing it. The current pipeline does not read `principal_` yet, but adding it costs ~12 bytes per patch and keeps the type signature aligned with the original. Dependents updated: - cpp/patchwork/include/patchwork/patchwork.h: delete duplicated PCAFeature/PatchStatus, include patchwork/types.h. - cpp/patchwork/src/patchwork.cpp: delete in-class xy2theta / xy2radius / estimate_plane, route call sites to the namespace-level helpers in patchwork/plane_fit.h. - cpp/patchworkpp/include/patchwork/patchworkpp.h: delete duplicated PointXYZ + xy2theta / xy2radius member decls, include patchwork/types.h. - cpp/patchworkpp/src/patchworkpp.cpp: delete its file-scope point_z_cmp and the PatchWorkpp::xy2theta / xy2radius definitions, include patchwork/plane_fit.h. `estimate_plane` is left untouched in patchworkpp.cpp (it still writes to instance-level pc_mean_/normal_/singular_values_) because refactoring it requires the thread-safety changes that will arrive with the TBB work in the next PR. CMake: new cpp/common/CMakeLists.txt builds ground_seg_common (static, PIC) linked against Eigen3::Eigen. Both classic and patchworkpp link to it. The patchworkpp build-tree export() set now also lists ground_seg_common to keep find_package(patchworkpp) usable from a build directory. Numerical equivalence verified on KITTI 00-10 full sweep (23,201 frames) for all four method × protocol combinations against the v1.3.1 baseline: every macro-average P / R / F1 cell within 0.0013 of the v1.3.1 number (two combinations byte-identical), well under the ±0.05 budget set in the refactor plan. Sets up the TBB parallelisation that will land in the follow-up PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three localised fixes to
cpp/patchwork/src/patchwork.cpp(the Patchwork reimplementation exposed aspypatchworkpp.patchwork) so it matches the original Patchwork on KITTI 00-10:elevation_thris converted to the sensor frame by subtractingsensor_height. The YAML thresholds in the original repo are documented as ground-frame; the reimpl was using the raw value, so the elevation gate effectively never fired for normal ground.normal . pagainstth_dist_d_ = th_dist - d_. The previous centred form shifted the cutoff by an extra-d_(~ 1.6 m on KITTI), which absorbed far-from-plane points into ground.elevation_thr.size()rings gets its own threshold. The previous(zone==0) ? ring : zonecollapse destroyed the per-ring tuning.Plus
USAGE.mdcovering:Results (KITTI 00-10, all 23,201 frames, paper-matched params:
uprightness_thr=0.707,using_global_thr=false)pypatchworkpp.patchwork, currentmainpypatchworkpp.patchwork, currentmainpypatchworkpp.patchwork, this PRpypatchworkpp.patchwork, this PR~/git/patchworkoriginal ROS 2 (reference)+2.29 F1 vs
mainunder the Patchwork++ paper protocol; within ±0.14 F1 of the original Patchwork ROS 2 build; within paper run-to-run variance of Table I.Ablation
500 frames/seq × 11 seqs = 5,500 frames per sweep on a per-fix branch state (compile-flagged before this PR squashed the toggles out). Patchwork++ paper protocol:
Fix 2 is the dominant win on the Patchwork++ paper protocol; Fix 1 contributes more on the Patchwork-paper protocol (+0.80 F1 alone). Fix 3 is small in isolation but logically required for
elevation_thr[2..3]to apply where the paper intended.Test plan
pip install -v ./python/from a clean conda env.pypatchworkpp.patchworkpp(the actual Patchwork++) unaffected — the three fixes are incpp/patchwork/, notcpp/patchworkpp/. Numbers identical before/after.master.References
elevation_thresholds).