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

Compute Forward Vector #276

Merged
merged 18 commits into from
Oct 4, 2024
Merged

Conversation

b-peri
Copy link
Collaborator

@b-peri b-peri commented Aug 22, 2024

Description (Edited 24/09/2024)

This PR adds compute_forward_vector() - a new kinematic function for computing a forward/rostral-facing vector given two keypoints - to movement. As described in #238, this function takes two left-right symmetrical keypoints (i.e. symmetrical across the animal's mid-saggital plane) and computes the vector perpendicular to the line connecting them. The result is an xr.DataArray of vectors pointing in the animal's rostral direction at each timepoint. As we expect this function to be commonly used for the computation of head direction vectors, this PR also adds compute_head_direction_vector(), a convenient alias for compute_forward_vector() whose docstring more explicitly refers to points on the head (rather than on the body, more generically).

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

References

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

@b-peri b-peri requested review from niksirbi and sfmig August 22, 2024 13:24
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.77%. Comparing base (9c80786) to head (268d395).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #276   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          15       15           
  Lines         887      909   +22     
=======================================
+ Hits          885      907   +22     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Thanks @b-peri! This is well implemented and well tested with the known values.

Some thoughts (apart from the things I point out in individual comments):

  1. would be nice to expand the tests a bit by checking for what happens if there is a NaN value. For example, you can make one of the ears NaN for a particular timepoint and test whether the head vector for that exact timepoint (and no other) is NaN.
  2. As we discussed today, you may move the new funciton to the kinematics module (anticipating our planned removal of the analysis level.
  3. Once you compelete the above, it would be good to use this function within the polar vectors example. For that you'd need to modify the code to use your function for computing the head vector (instead of midpoint + snout) and update the plots accordingly. I think it's worth doing that in this PR, as that example will serve as the perfect documentation for the new feature. Btw, becasue the example scripts are broken down into cells (via # %%), you can run them independently via ipython, which is helpful for debugging and editing that script. Let me know if you need help with that.

movement/analysis/navigation.py Outdated Show resolved Hide resolved
movement/analysis/navigation.py Outdated Show resolved Hide resolved
movement/analysis/navigation.py Outdated Show resolved Hide resolved
movement/analysis/navigation.py Outdated Show resolved Hide resolved
movement/analysis/navigation.py Outdated Show resolved Hide resolved
tests/test_unit/test_navigation.py Outdated Show resolved Hide resolved
tests/test_unit/test_navigation.py Outdated Show resolved Hide resolved
tests/test_unit/test_navigation.py Outdated Show resolved Hide resolved
tests/test_unit/test_navigation.py Outdated Show resolved Hide resolved
tests/test_unit/test_navigation.py Outdated Show resolved Hide resolved
sfmig
sfmig previously requested changes Sep 10, 2024
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.

sorry for the delay @b-peri !

From the longer discussion in zulip, I detail here what I think would be the simplest implementation.

I also added some comments about the tests, let me know if you have any questions. The basic idea is to split them based on their goals (e.g. split them if they test the error-catching, the behaviour with nans, or the 'ideal' behaviour) and parametrise them when possible.

Let me know if you'd like to discuss anything offline or if you have questions!

movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
@lochhh
Copy link
Collaborator

lochhh commented Sep 16, 2024

Quality Gate Passed Quality Gate passed

Issues 3 New issues 0 Accepted issues

Measures 0 Security Hotspots 0.0% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarCloud

Hey @b-peri , just a note to also remove any uppercase letters in function names.

@b-peri
Copy link
Collaborator Author

b-peri commented Sep 16, 2024

Quality Gate Passed Quality Gate passed

Issues 3 New issues 0 Accepted issues
Measures 0 Security Hotspots 0.0% Coverage on New Code 0.0% Duplication on New Code
See analysis details on SonarCloud

Hey @b-peri , just a note to also remove any uppercase letters in function names.

Hi @lochhh, thanks for the heads-up, that's my bad! The function names are fixed now!

@b-peri b-peri changed the title Compute Head Direction Vector Compute Forward Vector Sep 24, 2024
@b-peri b-peri mentioned this pull request Oct 3, 2024
7 tasks
Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Thank you @b-peri for implementing the consensus coming of our long-winded conversations.

I have nothing to comment on the implementation iself, or the tests, which are excellent and comprehensive. I left some docstring-formatting-related comments which you may take or leave as you wish.

Only remaining issue

There are issue with the polar example though:

The arrows have disappeared form one of the plots:
Screenshot 2024-10-04 at 10 33 48

Moreover, I realised that this example will need much more comprehensive updates, because Sofia had performed some downstream analyses which assume that the "head_vector" was a midpoint-to-snout vector, with meaningful magnitude. This is no longer the case.

My proposed way forward

  • Implement any of my formatting suggestions you like
  • undo the changes you've made in the sphinx example notebook
  • open an issue about updating said notebook in a separate PR
  • merge 🎉

movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
movement/analysis/kinematics.py Outdated Show resolved Hide resolved
tests/test_unit/test_kinematics.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Oct 4, 2024

@b-peri b-peri dismissed sfmig’s stale review October 4, 2024 14:55

All requested changes have now been implemented!

@b-peri b-peri added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 4, 2024
@b-peri b-peri added this pull request to the merge queue Oct 4, 2024
Merged via the queue into neuroinformatics-unit:main with commit a6dc15e Oct 4, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compute Head Direction and Angular Head Velocity
4 participants