icp-log-viewer: add --min-quality filter CLI flag#52
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a CLI option Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Parser
participant Main as main_show_gui()
participant Loader as LogRecord Loader
participant FileList as Filtered Files
User->>CLI: Launch with --min-quality
CLI->>Main: Provide minQ
Main->>Main: Validate 0.0 <= minQ <= 1.0
loop for each candidate file
Main->>Loader: LoadFromFile(candidate)
alt Load fails
Loader-->>Main: error
Main->>Main: Log warning, skip file
else Load succeeds
Loader-->>Main: LogRecord (icpResult.quality)
Main->>Main: Compare quality >= minQ?
alt Passes
Main->>FileList: Add file
else Fails
Main->>Main: Log skip to stderr
end
end
end
Main->>Main: Print "kept N / total" summary
alt No files kept
Main->>Main: Throw exception mentioning --min-quality
end
Main->>Main: Use filtered FileList for logRecords population
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/icp-log-viewer/main.cpp`:
- Around line 245-246: Validate the CLI value returned by
argMinQuality.getValue() before using it: read the value into minQ, check that
minQ is within [0.0, 1.0], and if not, print a clear error and exit non-zero (or
throw) instead of proceeding; update the std::cout line to only run after the
check. Reference argMinQuality.getValue(), the local variable minQ, and the
existing print/exit path in main (where the current std::cout lives) when adding
the range check and error handling.
- Around line 270-273: After applying the quality filter (where filteredFiles is
moved into files), check if files.empty() and handle that case with a clear
user-facing failure instead of letting execution hit
ASSERT_(!logRecords.empty()). Specifically, in the block around the use of
filteredFiles/files (the code that prints "Quality filter: kept ..."), detect
when files.empty(), print a helpful error message explaining that no files
passed the quality filter, and exit gracefully with a non-zero status (or return
an error) so downstream asserts are never triggered.
- Around line 251-268: LoadFromFile() doesn't throw on failure and returns a
default LogRecord, so stop relying on the try/catch and instead call the
lower-level load_from_file() that returns a bool; if
load_from_file(file.wholePath, out_lr) returns false, write the warning using
file.wholePath and file.name and skip pushing to filteredFiles, otherwise use
out_lr.icpResult.quality to compare with minQ and push to filteredFiles. Locate
references to LogRecord::LoadFromFile, load_from_file(), lr.icpResult.quality,
filteredFiles, file.wholePath and file.name to implement the change.
In `@docs/source/app_icp-log-viewer.rst`:
- Around line 132-134: The docs incorrectly suggest using --min-quality 0.0 to
inspect low-quality registrations; --min-quality is a lower bound and 0.0 will
load everything. Update the explanatory text and the example command to use the
correct flag and value for low-quality filtering (e.g., change the comment and
command to mention inspecting only registrations with quality <20% by using
--max-quality 0.2 with the icp-log-viewer command), and ensure the description
refers to --max-quality rather than --min-quality.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b0c3d67-a3de-4844-a58e-357434b9be82
📒 Files selected for processing (2)
apps/icp-log-viewer/main.cppdocs/source/app_icp-log-viewer.rst
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #52 +/- ##
===========================================
- Coverage 78.60% 78.60% -0.01%
===========================================
Files 190 190
Lines 10573 10572 -1
Branches 977 977
===========================================
- Hits 8311 8310 -1
Misses 2262 2262 🚀 New features to boost your workflow:
|
jlblancoc
force-pushed
the
icp-log-viewer-quality-filter
branch
from
541db35 to
0e0e542
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/icp-log-viewer/main.cpp (1)
274-281:⚠️ Potential issue | 🟠 MajorCheck emptiness after applying the filter result, not before.
files.empty()is evaluated beforefilesis replaced withfilteredFiles, so the “no files passed” case is still missed and later hitsASSERT_(!logRecords.empty()).Proposed fix
- if (files.empty()) - { - THROW_EXCEPTION_FMT( - "No log files passed --min-quality=%.03f. Lower the threshold or check input logs.", - minQ); - } - files = std::move(filteredFiles); + if (files.empty()) + { + THROW_EXCEPTION_FMT( + "No log files passed --min-quality=%.03f. Lower the threshold or check input logs.", + minQ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/icp-log-viewer/main.cpp` around lines 274 - 281, The check for empty input uses files.empty() before files is replaced with filteredFiles, so the “no log files passed” path can be missed; change the logic in main (the block around files, filteredFiles, minQ) to first assign files = std::move(filteredFiles) and then call THROW_EXCEPTION_FMT when files.empty(), i.e. evaluate emptiness after the move so the minQ-based filtering result is honored. Ensure you reference the same variables (files, filteredFiles, minQ) and keep the error text unchanged.docs/source/app_icp-log-viewer.rst (1)
123-134:⚠️ Potential issue | 🟡 MinorThe low-quality example is still misleading.
--min-quality 0.0does not isolate low-quality runs; it effectively loads all files.Proposed fix
-When a directory contains many log files (e.g. from a full SLAM session) it is often -useful to inspect only the low-quality registrations that may have caused drift. Use -``--min-quality`` to restrict the loaded set: +When a directory contains many log files (e.g. from a full SLAM session), use +``--min-quality`` as a lower-bound filter to focus on higher-confidence results: @@ - # Inspect only low-quality (< 20 %) registrations — invert by running with 0 - # and then sorting/filtering externally, or set a low threshold and browse manually + # Note: --min-quality is a lower bound. Using 0.0 loads all files. + # To inspect low-quality-only runs, post-filter externally. icp-log-viewer -d logs/ --min-quality 0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/app_icp-log-viewer.rst` around lines 123 - 134, The docs example is misleading: `--min-quality 0.0` does not isolate low-quality runs (it loads all files). Update the second example and its explanatory text to show the correct flag/approach for selecting low-quality registrations (e.g., use `--max-quality 0.2` to inspect <20% runs) and remove or reword the suggestion about inverting by running with 0; reference the CLI flags `--min-quality` and `--max-quality` and the `icp-log-viewer` example lines so the sample and description match actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/icp-log-viewer/main.cpp`:
- Around line 80-82: The CLI help string passed to the ValueArg for the ICP
quality threshold contains doubled percent signs ("0%%-100%%"); update the
literal to use single percent characters ("0%-100%") so the help displays
"0%-100%" correctly; locate the ValueArg construction that includes the text
"Minimum ICP quality..." in main.cpp and replace the doubled "%%" occurrences in
that help string with single "%" characters.
---
Duplicate comments:
In `@apps/icp-log-viewer/main.cpp`:
- Around line 274-281: The check for empty input uses files.empty() before files
is replaced with filteredFiles, so the “no log files passed” path can be missed;
change the logic in main (the block around files, filteredFiles, minQ) to first
assign files = std::move(filteredFiles) and then call THROW_EXCEPTION_FMT when
files.empty(), i.e. evaluate emptiness after the move so the minQ-based
filtering result is honored. Ensure you reference the same variables (files,
filteredFiles, minQ) and keep the error text unchanged.
In `@docs/source/app_icp-log-viewer.rst`:
- Around line 123-134: The docs example is misleading: `--min-quality 0.0` does
not isolate low-quality runs (it loads all files). Update the second example and
its explanatory text to show the correct flag/approach for selecting low-quality
registrations (e.g., use `--max-quality 0.2` to inspect <20% runs) and remove or
reword the suggestion about inverting by running with 0; reference the CLI flags
`--min-quality` and `--max-quality` and the `icp-log-viewer` example lines so
the sample and description match actual behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59556ed0-3486-43b5-8412-eb567a8b8333
📒 Files selected for processing (2)
apps/icp-log-viewer/main.cppdocs/source/app_icp-log-viewer.rst
jlblancoc
force-pushed
the
icp-log-viewer-quality-filter
branch
from
0e0e542 to
a59cfe4
Compare
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.
Summary by CodeRabbit
New Features
Documentation