Add tests for heading angle #385
Merged
Conversation
This file contains 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
* Implement the expected fix updates: - [github.com/astral-sh/ruff-pre-commit: v0.8.1 → v0.8.6](astral-sh/ruff-pre-commit@v0.8.1...v0.8.6) - [github.com/pre-commit/mirrors-mypy: v1.13.0 → v1.14.1](pre-commit/mirrors-mypy@v1.13.0...v1.14.1) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Revert "Implement the expected fix" This reverts commit edbaa9c. * Update method to explicitly construct and drop spatial z dimensions * Force nan test to check for preserved coordinates * Force explicit coordinate preservation checks in input/output test --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ns-heading-angle #385 +/- ##
====================================================
+ Coverage 99.79% 99.80% +0.01%
====================================================
Files 14 14
Lines 969 1025 +56
====================================================
+ Hits 967 1023 +56
Misses 2 2 ☔ View full report in Codecov by Sentry. |
7 tasks
niksirbi
approved these changes
Jan 27, 2025
There was a problem hiding this comment.
Very well done @willGraham01!
I'm happy with this implementation and I'm in awe of how thoroughly and creatively you've tested the functionality.
I've mostly just pointed out some pedantic naming issues and some Sphinx rst-related formatting issues. Feel free to adopt or push back.
Co-authored-by: Niko Sirmpilatze <[email protected]>
|
7 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 28, 2025
* Basic implementation of `compute_heading()` and `compute_relative_heading()` * Minor fixes and docstring edits * Remove `compute_relative_heading()` from this PR * Created `signed_angle_between_2d_vectors()` vector util and refactored `compute_heading()` * Cleaned up redundant code * get rid of analysis folder * add exact_coords boolean to the validator for dims and coords * adapted reference vector validator * renamed compute_heading to compute_heading_angle * fixed docstring formatting errors * renamed new vector util to compute_signed_angle_2d * Add tests for heading angle (#385) * Breakout vector valiadator with skeleton test * Write validator tests, fix bug in missing .get method * Fix dims and coords returned by `compute_forward_vector` (#382) * Implement the expected fix updates: - [github.com/astral-sh/ruff-pre-commit: v0.8.1 → v0.8.6](astral-sh/ruff-pre-commit@v0.8.1...v0.8.6) - [github.com/pre-commit/mirrors-mypy: v1.13.0 → v1.14.1](pre-commit/mirrors-mypy@v1.13.0...v1.14.1) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Revert "Implement the expected fix" This reverts commit edbaa9c. * Update method to explicitly construct and drop spatial z dimensions * Force nan test to check for preserved coordinates * Force explicit coordinate preservation checks in input/output test --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Standardise compute_signed_angle_2d function as a purely mathematical impementation * Patch up compute_forward_vector_angle * Start on tests but encounter NaN bug that needs rebase fix * Fix mapping of -pi to pi in signed_angle * Write antisymmetry test * Write invariance under transforms test * Add tests for missing lines * Apply batch suggestions from code review Co-authored-by: Niko Sirmpilatze <[email protected]> * Fix computer_forward_vector__angle docstring * Remove unnecessary validation checks * Use camera view variable name for codebase consistency * Fix validator docstring backticks * Move ref vector validator into array validators * Fix typehints and test broken by removing earlier error check --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Niko Sirmpilatze <[email protected]> --------- Co-authored-by: b-peri <[email protected]> Co-authored-by: Will Graham <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
What is this PR
Why is this PR needed?
Builds on top of #376, adding the necessary tests as well as reorganising the functionality that the base branch intends to introduce.
What does this PR do?
validate_reference_vectorfunctioncompute_signed_angle_2dfunction, turning it into a mathematical implementation that is designed to be wrapped by higher-level, user-friendly functions.compute_forward_vector_angle(calledcompute_heading_anglein the base branch) to accommodate the possibility of different conventions for the angle sign.References
Builds on #376, with a view to merging the quoted PR after this one.
How has this PR been tested?
Is this a breaking change?
No.
Does this PR require an update to the documentation?
No.
Checklist: