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

Running speed incorrectly calculated. #1741

Closed
colleenjg opened this issue Oct 8, 2020 · 3 comments
Closed

Running speed incorrectly calculated. #1741

colleenjg opened this issue Oct 8, 2020 · 3 comments
Labels
braintv relates to Insitute BrainTV program bug

Comments

@colleenjg
Copy link
Contributor

colleenjg commented Oct 8, 2020

Describe the bug
2 errors introduced by latest commit to allensdk/brain_observatory/extract_running_speed/__main__.py in extract_running_speeds().

  1. The if/else statements (line 62-65) are reversed: median duration is not used if use_median_duration is True, and is used otherwise.
  2. In the else statement, angular_velocity is accidentally set to the median duration due to a = instead of a / on line 65. As a result, the running velocity array is set entirely to a single repeating value, namely the median duration.

allensdk version 2.3.0


FIX:

if use_median_duration:
    angular_velocity = dx_rad / durations
else:
    angular_velocity = dx_rad = np.median(durations)

should be

if use_median_duration:
    angular_velocity = dx_rad / np.median(durations)
else:
    angular_velocity = dx_rad / durations
@colleenjg colleenjg added the bug label Oct 8, 2020
@kschelonka
Copy link
Contributor

kschelonka commented Oct 8, 2020

Hi Colleen, thanks for the report. Looks like we overlooked this module in our update.

Edit:
@colleenjg Can you clarify how recent commits have affected this? I don't see any changes in the past year, or any dependencies on the running speed code that was updated recently. I agree that what you're describing are bugs but it seems like they're longstanding, not recent.

@kschelonka
Copy link
Contributor

Regardless, this seems pretty cut and dry to me. You're welcome to make a PR if you'd like, otherwise we'll add it to our sprint schedule for fixing. 👍

@wbwakeman wbwakeman added the braintv relates to Insitute BrainTV program label Oct 8, 2020
@colleenjg
Copy link
Contributor Author

@kschelonka right, that wasn't very clear. I meant that the bug was introduced in the most recent commit where this specific script was modified. But you are right, that commit was made back in Sept 2019, so it is a longstanding bug.
And great, I'll make a PR ASAP :)

colleenjg added a commit to colleenjg/AllenSDK that referenced this issue Oct 13, 2020
Fixes 2 errors in allensdk.brain_observatory.extract_running_speed.__main__.py
in `extract_running_speeds()`. In the if/else statement (lines 61-65),
1) the second consecutive `=` (a typo) was corrected to `\` so that `dx_rad` is
divided by median duration. 2) The if/else clauses were switched, so that the if
`use_median_duration` statement is the one that uses np.median(durations) and not v.v.
kschelonka added a commit to colleenjg/AllenSDK that referenced this issue Oct 13, 2020
kschelonka added a commit that referenced this issue Oct 15, 2020
…main

Fixes 2 errors in allensdk.brain_observatory.extract_running_speed.__…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
braintv relates to Insitute BrainTV program bug
Projects
None yet
Development

No branches or pull requests

3 participants