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.

Sign up for GitHub

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

Jump to bottom

Add functionality for computing ego- and allocentric angles to RoIs #416

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Add functionality for computing ego- and allocentric angles to RoIs #416

wants to merge 34 commits into from
+1,425 −92

Conversation

willGraham01
Copy link
Contributor

@willGraham01 willGraham01 commented Feb 13, 2025 •
edited
Loading

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Introduces features necessary for computing ego- and allocentric boundary angles.

What does this PR do?

See the discussion in 370 for disambiguation of terms.

Several related methods are added to the RoI class. Most of these are self-evident from the name.

  • compute_distance_to
  • compute_nearest_point_to
  • compute_approach_vector
  • compute_allocentric_angle
  • compute_egocentric_angle

These methods are added to the base region of interest class, so they can be used by all regions of interest.

In addition, LineOfInterest has also been given two additional methods;

  • normal - returns the normal to the segment / line
  • compute_angle_to_support_plane_of_segment - this performs the egocentric calculation, but using the normal vector $\vec{n}$ rather than the approach vector $\vec{a}$.

NOTE: There are a lot of features going in here which are interconnected, but the PR diff is still quite large. It is possible to break the addition of compute_distance_to and compute_nearest_point_to out into its own PR without too much difficultly, but all the other methods are very much interconnected and need to come in at the same time.

References

This functionality should close both #368 and #370.

In particular, we now have the functionality to do any of the calculations discussed in #370. This includes using the normal vector in place of the approach vector when considering a segment (compute_angle_to_support_plane_of_segment). Polygonal regions do not have this option, due to this behaviour being ill-defined at corners, but users can access the individual segments that make up the polygon's boundary, which are LineOfInterests, and then call compute_angle_to_support_plane_of_segment on the segment they deem appropriate.

How has this PR been tested?

Addition of tests to the test suite.
Dependence on shapely methods has been preferred throughout.

Is this a breaking change?

Does this PR require an update to the documentation?

Could potentially give more scope to the examples that are made as part of #415.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@codecov Codecov

This comment was marked as resolved.

Sign in to view
@willGraham01 willGraham01 force-pushed the wgraham-370-368-roi-vectors branch from 0bf884f to df84664 Compare February 17, 2025 12:19
@willGraham01 willGraham01 marked this pull request as ready for review February 18, 2025 12:10
@willGraham01 willGraham01 mentioned this pull request Feb 18, 2025
Open
This was linked to issues Feb 18, 2025
Compute distance between a point and a line #368
Open
Compute angle between a vector and a plane perpendicular to the image plane #370
Open
sfmig
sfmig requested changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@sfmig sfmig left a comment

Choose a reason for hiding this comment

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

great job consolidating all this work @willGraham01 !

The vast majority of my comments are about simplifying the function signatures, renaming variables to more intuitive names, and clarifying docstrings.

Re the implementation, I spotted an error with how the normal to a plane is computed. This is currently not captured by the tests because we test with a segment starting at the origin - I explain and provide examples in the comments. For the rest, it mostly looks good and clear to me. I mainly commented on things that may reduce the complexity (for example, removing _vector_from_centroid_of_keypoints or fixing some conventions to keep the function signatures short).

I have two additional more general comments:

  • I think we should review these new ROI additions and make sure we highlight to the users when we assume the camera is top-down/bottom-up - this may be out of the scope of this PR though but something to keep in mind.
  • We should think or check with the researchers (e.g. Sepi) what offset in angle with respect to a plane makes more sense in terms of interpretation. For example, if the head vector is at 0 deg wrt a wall or boundary - would that be interpreted as facing the wall? or is the animal's back to the wall? Not a big issue but maybe worth discussing. Right now, the animal is at 0deg when it has its back (of the head) to the wall.

I tagged @niksirbi in a few of the comments to get his view (and any other comments on the rest are more than welcome too). But I think to avoid duplicate effort it would be most useful if he reviews this PR after the suggested changes are reviewed. I also focused much less on the tests, so maybe that is something he can focus on more next.

@sonarqubecloud SonarQubeCloud

This comment was marked as resolved.

Sign in to view
@willGraham01 willGraham01 requested a review from sfmig February 21, 2025 16:09
@willGraham01
Copy link
Contributor Author

willGraham01 commented Feb 21, 2025 •
edited
Loading

@sfmig @niksirbi I think I've captured everything caught in the initial review and as discussed in the community call today.

Key changes that are more than just renames / reorganisations:

  • We use the boundary_only keyword argument to most methods now, and the purpose of this keyword argument is explain in full in the class-docstring, to avoid having to repeat this explanation in every method.
  • The angle-computing methods now just take a forward_vector directly, rather than computing it from a single dataset, as per @sfmig's recommendation to reduce function signature complexity and avoid having to handle complications with the camera view, etc.
  • The approach vector is now defined as always being directed towards a region from another point. There is no toggle for this option in the various methods.
  • The signed angles returned from boundary-vector / angle computations CAN still be toggled via an explicit control, but the default adheres to a single convention (we always take the signed angle between the approach/normal and the forward/reference).
  • Some internal methods (notably _boundary_angle_computation have been added to the BaseRegionOfInterest class to avoid code duplication where possible.

Also, tests are failing due to an internal server error fetching sample data in a place unrelated to this PR. Will re-run in a bit to give the server a chance 😅

sfmig
sfmig reviewed Feb 21, 2025
View reviewed changes
movement/roi/base.py
return angles

@staticmethod
def _reassign_space_dim(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!! ✨

@niksirbi
Copy link
Member

Sounds great! Thanks both, I will do my own pass over this PR and try to finalise my review by end of Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfmig sfmig sfmig requested changes

@niksirbi niksirbi Awaiting requested review from niksirbi

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@willGraham01 @niksirbi @sfmig