Skip to content

Add RPE computation for different segment lengths, add basic evaluation example - #135

Open
mitchellcohen3 wants to merge 1 commit into
mainfrom
add_evaluation_example
Open

Add RPE computation for different segment lengths, add basic evaluation example#135
mitchellcohen3 wants to merge 1 commit into
mainfrom
add_evaluation_example

Conversation

@mitchellcohen3

Copy link
Copy Markdown
Member

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds trajectory evaluation utilities (ATE + RPE over multiple segment lengths) and a runnable example demonstrating how to compare multiple SLAM/odometry estimates on a dataset.

Changes:

  • Introduces navlie.utils.evaluation with ATE computation, RPE computation over configurable segment lengths, and an RPE boxplot helper.
  • Adds examples/ex_evaluation.py to demonstrate loading trajectories, simulating drift, computing metrics, and plotting results.
  • Updates README Python version badge.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.

File Description
navlie/utils/evaluation.py New evaluation + plotting helpers built on evo and seaborn/pandas.
examples/ex_evaluation.py New end-to-end example computing ATE/RPE and plotting trajectories + RPE distributions.
README.rst Updates the Python version badge range.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +180 to +186
df = pd.DataFrame(rows)

if ax is None:
fig, ax = plt.subplots(figsize=figsize)
else:
fig = ax.get_figure()

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plot_rpe_boxplot assumes the dataframe is non-empty (e.g., df["segment_length"]), but compute_rpe_over_segment_lengths can return empty arrays on failure, which can lead to rows=[] and an empty dataframe. In that case this will raise a KeyError. Consider handling the empty/no-data case explicitly (e.g., early return with an empty plot or a clear error).

Suggested change
df = pd.DataFrame(rows)
if ax is None:
fig, ax = plt.subplots(figsize=figsize)
else:
fig = ax.get_figure()
df = pd.DataFrame(rows, columns=["Estimator", "segment_length", "error"])
if ax is None:
fig, ax = plt.subplots(figsize=figsize)
else:
fig = ax.get_figure()
if df.empty:
ax.set_xlabel(xlabel)
ax.set_ylabel(ylabel)
fig.tight_layout()
return fig, ax

Copilot uses AI. Check for mistakes.
Comment thread examples/ex_evaluation.py
Comment on lines +79 to +82
gt_states, est_states_aligned, _ = associate_and_align_trajectories(
gt_states,
est_states,
)

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside the estimator loop, gt_states is reassigned from associate_and_align_trajectories(...). This means the second iteration no longer uses the original full ground-truth trajectory, which can change both plotting and metrics depending on association results of the first estimate. Use a separate variable (e.g., gt_states_aligned/gt_states_sync) so each estimate is associated against the same original groundtruth.

Copilot uses AI. Check for mistakes.
Comment thread examples/ex_evaluation.py
Comment on lines +93 to +113
ate_att, ate_pos = compute_ate(
gt_states,
est_states,
align=True,
)
ate_dict_att[f"estimate_{i+1}"] = ate_att
ate_dict_pos[f"estimate_{i+1}"] = ate_pos

# Compute RPE over different segment lengths
rpe_att = compute_rpe_over_segment_lengths(
gt_states,
est_states,
align=True,
pose_relation=PoseRelation.rotation_angle_deg,
)
rpe_pos = compute_rpe_over_segment_lengths(
gt_states,
est_states,
align=True,
pose_relation=PoseRelation.translation_part,
)

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example aligns trajectories via associate_and_align_trajectories(...) but then computes ATE/RPE using est_states (unaligned) while also passing align=True into compute_ate / compute_rpe_over_segment_lengths. This duplicates work and makes it unclear which alignment the reported metrics correspond to. Consider either (a) computing metrics on est_states_aligned with align=False, or (b) removing the explicit alignment step and relying on the metric functions for alignment (keeping a separate aligned copy only for plotting if needed).

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +21
def compute_ate(
gt_states: typing.List[SE3State],
est_states: typing.List[SE3State],
max_diff: float = 0.02,
align: bool = True,

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New evaluation utilities are introduced here without accompanying unit tests. Since the repo already has a pytest suite, consider adding basic tests (e.g., identical trajectories -> ~0 ATE/RPE; synthetic drift -> RPE arrays non-empty) to validate the evo integration and prevent regressions.

Copilot uses AI. Check for mistakes.
Comment on lines +190 to +194
# Optional boxplot kwargs
kwargs = dict(
fill=False,
width=0.8,
gap=0.15,

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sns.boxplot is being called with fill and gap defaults, but the package dependency is seaborn>=0.11.2 (setup.py). These kwargs are not supported in older seaborn versions and will raise a TypeError for users pinned near the minimum. Either remove/avoid these kwargs (use only options available in 0.11.x), or bump the seaborn minimum version accordingly so the API matches.

Suggested change
# Optional boxplot kwargs
kwargs = dict(
fill=False,
width=0.8,
gap=0.15,
# Optional boxplot kwargs. Keep defaults compatible with seaborn>=0.11.2.
kwargs = dict(
width=0.8,

Copilot uses AI. Check for mistakes.
Comment on lines +157 to +158
title: str
Plot title.

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring lists a title: str parameter, but the function signature doesn't accept title. This is misleading for users; either add the parameter and apply it (e.g., ax.set_title(...) / fig.suptitle(...)) or remove the docstring entry.

Suggested change
title: str
Plot title.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants