Timestamp issue - #5
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new RMSE alignment logic hard-fails on duplicate GT timestamps (which other repo scripts already anticipate) and the PR description claims regression tests were added but none are included in the change set.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses Issue #4 by switching linear-velocity RMSE alignment from unsafe row-index pairing to timestamp-based alignment, using estimator absolute timestamps and interpolating GT velocities at estimator timestamps.
Changes:
- Prefer
t_abswhen selecting a timestamp column and fail fast if no timestamp column is present. - Replace index-based alignment with GT→EST timestamp interpolation for velocity RMSE computation.
- Extend
.gitignoreto ignore additional exported artifact types (*.pdf,*.tum).
File summaries
| File | Description |
|---|---|
data_process/scripts/compute_vel_rmse.py |
Enforces timestamp presence and aligns velocity samples via timestamp interpolation instead of index-based pairing. |
.gitignore |
Ignores additional export artifact extensions (.pdf, .tum). |
Review details
- Files reviewed: 1/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| gt_sorted = sorted(gt, key=lambda r: r[0]) | ||
| est_sorted = sorted(est, key=lambda r: r[0]) | ||
| gt_times = [r[0] for r in gt_sorted] | ||
|
|
||
| n = min(len(gt), len(est)) | ||
| return gt[:n], est[:n], "index" | ||
| if any(t1 >= t2 for t1, t2 in zip(gt_times, gt_times[1:])): | ||
| raise ValueError("GT timestamps must be unique") |
| if not gt_aligned: | ||
| raise ValueError("GT and EST timestamp ranges do not overlap") | ||
|
|
||
| return gt_aligned, est_aligned, "timestamp interpolation (GT -> EST)" |
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly address the reported timestamp misalignment by enforcing timestamp columns and implementing deterministic GT→EST interpolation without introducing evident correctness or API issues.
Review details
- Files reviewed: 1/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change directly addresses the reported misalignment by enforcing timestamp-based alignment with interpolation and is limited in scope to the evaluation script.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
This PR is to solve this issue.
Fix: align velocity RMSE samples by timestamp
Prefer absolute timestamps from fused-state outputs and interpolate ground-truth velocities at estimator timestamps. Remove the unsafe index-based fallback.