-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix dims and coords returned by compute_forward_vector
#382
Conversation
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>
This reverts commit edbaa9c.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #382 +/- ##
=======================================
Coverage 99.79% 99.79%
=======================================
Files 14 14
Lines 969 969
=======================================
Hits 967 967
Misses 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the quick bug fix and the very thorough explanation @willGraham01!
Previously this test did not actually check if the returned array was empty (which it was), so the np.isnan(...).all() and not np.isnan(...).any() were comparing to the empty container, this always returning True.
Thanks for checking this, it makes sense now.
LGTM, barring an optional comment. Feel free to merge at your leisure.
I will make a patch release after this PR is merge.
* 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]>
* 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>
Description
What is this PR
Why is this PR needed?
See #381
What does this PR do?
The function concerned,
compute_forward_vector
, has been changed to now explicitly construct theupwards_vector
with appropriate labelling, and then the result has the extra spatial dimension dropped to ensure we always drop the correct axis.The existing unit tests have been given a similar treatment.
nan
values has been reworked to use the more precisesel
andisnull
method(s) to pull expected / unexpectedNaN
values out of the result, check that the appropriatetime
,space
, andindividuals
axes are preserved, AND to force a comparison of the shape of the sub-array that containsNaN
values against that which is expected. Previously this test did not actually check if the returned array was empty (which it was), so thenp.isnan(...).all()
andnot np.isnan(...).any()
were comparing to the empty container, this always returningTrue
.References
Closes #381
How has this PR been tested?
Local test suite passes, both prior to and with the additions mentioned above.
In the case of the old
nan
test, the old version of the test passed with the updates to the code (though this was to be expected due to the aforementioned empty-array bug), before being replaced.Is this a breaking change?
No
Does this PR require an update to the documentation?
If any features have changed, or have been added. Please explain how the
documentation has been updated.
Checklist: