Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several minor bugs in plot_trajectory #417

Closed
niksirbi opened this issue Feb 14, 2025 · 3 comments · Fixed by #425
Closed

Several minor bugs in plot_trajectory #417

niksirbi opened this issue Feb 14, 2025 · 3 comments · Fixed by #425
Assignees
Labels
bug Something isn't working

Comments

@niksirbi
Copy link
Member

This issue highlights two related problems with the current plot_trajectory function: ambiguous or incorrect plot titles, and mismatched time/space units. Some of this may be also relevant for the plot_occupancy function that's coming with #403.

1. Misleading Titles

When plotting multiple trajectories on the same axis (as demonstrated here), the final plot title is determined by the last individual plotted. This can lead to confusion when multiple trajectories are shown.

In certain other edge cases, the title ends up displaying "Trajectory of None".

Proposed Fix
Rather than trying to infer a suitable title based on the plotted data, we should either:

  • Omit the title by default.
  • Use a generic title that's always true (e.g., "Trajectory").

Users can always override the title themselves if needed.

2. Erroneous Time/Space Units

The plot_trajectory() function attempts to read space_unit and time_unit attributes directly from the position DataArray. However, these attributes are only set at the Dataset level, meaning plot_trajectory() defaults to "pixels" and "frames" even if the actual units are, for example, "meters" and "seconds".

Possible Solutions

  1. Pass the entire Dataset
    This ensures that plotting functions have access to dataset-level attributes. However, it complicates matters when a dataset contains multiple variables (e.g., position, confidence) and forces users to carry unneeded data.

  2. Inherit attributes in the DataArray
    On dataset creation or file loading, propagate relevant attributes (e.g., time_unit, space_unit) to each DataArray. This requires deeper changes in the I/O layer and consideration of broader package implications.

  3. Stop attempting to infer units
    As a quick hotfix, simply label plot axes as "x", "y", and "Time" without specifying units. This avoids misleading labels until we have a more comprehensive solution.

Recommendation
I propose implementing Option 3 for now, allowing a functional v0.0.24 release. A more complete fix (Option 2) can be pursued later once we have time to address the underlying I/O and attribute-inheritance logic.

@niksirbi niksirbi added the bug Something isn't working label Feb 14, 2025
@niksirbi niksirbi moved this from 🤔 Triage to 📝 Todo in movement progress tracker Feb 14, 2025
@niksirbi
Copy link
Member Author

I'm probably the one to blame fro the 2nd bug, because I encouraged relying on space_unit and time_unit to begin with, not realising that these are not defined on the position data array level.

@stellaprins stellaprins self-assigned this Feb 20, 2025
@stellaprins stellaprins linked a pull request Feb 20, 2025 that will close this issue
7 tasks
@stellaprins
Copy link
Contributor

Thanks @niksirbi. Used your suggestions and double checked the plot_occupancy function that's coming with #403 but this doesn't seem to have the issues.

I did not see plot_occupancy used in any of the examples and was tempted to add a plot, like the one below, to Express 2D vectors in polar coordinates (but it's probably better to have a seperate plot example than to bloat the current examples!).

Image

@github-project-automation github-project-automation bot moved this from 📝 Todo to ✅ Done in movement progress tracker Feb 20, 2025
@niksirbi
Copy link
Member Author

I did not see plot_occupancy used in any of the examples and was tempted to add a plot, like the one below, to Express 2D vectors in polar coordinates (but it's probably better to have a seperate plot example than to bloat the current examples!).

I am currently working on updating the polar coordinate example #317 , and I also want to tackle #410 in combination with #415, which should take care of showcasing the occupancy plot. So don't worry about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

2 participants