Extract road from lidar scan - #34
Merged
Merged
Conversation
1. Critical Off-by-One Bug in Sliding Window Logic
Both slow_get_best_match and the vectorized get_best_match implementations contain an identical off-by-one error. They
skip the very last possible window of the array.
* In slow_get_best_match: The loop for i in range(0, len(mask) - window_size): stops one element early. It should be
range(0, len(mask) - window_size + 1).
* In get_best_match: The window_sums array allocation np.empty(len(mask) - window_size) is one element too small.
Additionally, the slice cum[window_size:-1] drops the last element. The correct vectorized form should be:
1 window_sums = np.empty(len(mask) - window_size + 1, dtype=cum.dtype)
2 window_sums[0] = cum[window_size - 1]
3 window_sums[1:] = cum[window_size:] - cum[:-window_size]
* Test Masking the Bug: The test_get_best_match passes because both the slow and fast implementations share this
exact same bug, so their outputs match. If you add a test case where the optimal window is exactly at the end of
the array (e.g., mask = [0, 0, 1, 1, 1] with window_size = 3), both algorithms will incorrectly return index 1
instead of 2.
2. Redundant Aliasing in __init__.py The lidarroad/__init__.py file uses explicit aliases like analyze_scan as analyze_scan. While this syntax is occasionally used to explicitly re-export symbols for strict type checkers (like pyright), this codebase doesn't use type hints. This makes the imports overly verbose. A clean from .lidarroad import analyze_scan, batch_processing, ... would be more idiomatic.
3. Missing Test Coverage for Window Boundary Edge Case In test_lidarroad.py, the test_get_best_match test loops over multiple arrays and window sizes but intentionally skips testing when the sizes are equal: if len(m) > window_size:. While the implementations do currently handle len(m) == window_size without crashing (they correctly return 0, window_size), it is best practice to remove that if constraint in the test to ensure this edge case remains explicitly covered and doesn't regress in the future.
Member
Author
|
OK, I am going to merge this as "version 0". In particular there is missing OSGAR Node, which would handle navigation, but as it will be part of "Dobyvani hradu 2026" integration, it should be mix of GPS, lidar, obstacle detection and maybe redroad, so could be more complex and hard to tell at the moment. Another "issue" is that the road width may change and it is not taken into account. Also if the window size is wrong then local minima does not have strong "anchor". With this I would play later once we collect new data on the Divci kamen castle :). |
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.
Find smooth section with given tolerance for expected road width