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

Add radius as conversion (scaling) instead of adding the scaled data in FicTrac #619

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

h-mayorquin
Copy link
Collaborator

This was a suggestion from @CodyCBakerPhD in our last conversion which I think makes sense. We were adding the data scaled which increases the error if the measurement of the radius is not very precise.

This method keeps both the radius, the right units and the original data without imprecisions.

I modified the test to account for this.

@h-mayorquin h-mayorquin marked this pull request as ready for review October 27, 2023 14:59
@CodyCBakerPhD
Copy link
Member

Elegant separation of source data and utilization of existing mechanisms/attributes on the neurodata classes!

I will mention one more thing for a future improvement, dependent on resolution of NeurodataWithoutBorders/pynwb#1766

For this kind of thing (any time there are radial + linearized interpretations of data) it is common to include two separate time series for each interpretation (each named clearly for easy identification), since even now the detail of the original radial data could be obscured by the conversion factor - the most common case of this is eye tracking, where we might include both the angle of the pupil relative to the target on the screen, and then the processed estimate of that x/y position given the angles - but this seems similar enough

This might seem like duplication, and it is, which is why I don't advocate for it right at the moment - but once the issue is resolved it should be possible to internally inter-link the data field of the two series so that the data is only written once

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #619 (7495597) into main (424ba80) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #619   +/-   ##
=======================================
  Coverage   91.25%   91.25%           
=======================================
  Files         106      106           
  Lines        5408     5408           
=======================================
  Hits         4935     4935           
  Misses        473      473           
Flag Coverage Δ
unittests 91.25% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...nterfaces/behavior/fictrac/fictracdatainterface.py 66.35% <100.00%> (ø)

@CodyCBakerPhD CodyCBakerPhD merged commit 9838b67 into main Oct 30, 2023
34 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the add_radius_as_scaling branch October 30, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants