Remove toggles for signed angle computations #445
+61
−208
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
- Coverage 99.84% 99.84% -0.01%
==========================================
Files 23 23
Lines 1312 1301 -11
==========================================
- Hits 1310 1299 -11
Misses 2 2 ☔ View full report in Codecov by Sentry. |
|
@niksirbi I can pick this up if you want |
Thanks! Happy for you to take charge of reviewing this. |
sfmig
approved these changes
Feb 28, 2025
There was a problem hiding this comment.
Looks great @willGraham01 ! 🚀
I just spotted a couple of references to old syntax, the other two are minor comments.
So nice to have this functionality 🤩
This comment was marked as resolved.
This comment was marked as resolved.
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?
See #438 - simplifies the call signature of functions that compute angles between vectors, and allows us to use precise language in our doc-strings when describing these quantities.
Also see #427.
Both of these issues were raised in #416, but were shifted to issues to reduce the PR complexity. Now that #416 is merged, we can apply these changes. Have tagged the original, hard-working reviewers from that PR here (but probably only one pair of eyes needed) 😅
What does this PR do?
Removes the option to toggle the "angle direction" in (high-level) functions that compute signed angles between two vectors, in some capacity. Specifically;
compute_forward_vector_angle- this function has also had it's default angle unit switched to radians (previously degrees) to address Switchcompute_forward_vector_angleto return radian angles by default #427,compute_angle_to_normal,compute_{ego,allo}centric_angle_to_nearest_point.The above functions already adhered to a consistent convention when their default values were used, which has been made the definition and standard.
The lower-level
compute_signed_angle_2dfunction, that these functions all depend on, has retained it'sv_as_left_operandargument. This is because the function is asymmetric in its arguments, and there may be cases where we really do want to swap the roles of the inputuandv.Small Test Suite Changes
One error-case test has been removed, since this is actually tested by
compute_signed_angle_2danyway - and in fact the error that is checked-for is raised within that function.A test that the
in_degreestoggle forcompute_forward_vector_anglehas been added, since CodeCov was complaining.References
Closes #427.
Closes #438.
How has this PR been tested?
Since this is just removal of a feature, the now-irrelevant parts of the test suite have been removed. All other tests still pass with the adoption of the new convention.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
No.
Checklist: