Skip to content

Conversation

@colleenjg
Copy link
Contributor

@colleenjg colleenjg commented Oct 12, 2020

Overview:

There are 2 errors in extract_running_speeds() in allensdk/brain_observatory/extract_running_speed/__main__.py.

  1. The if/else statements at lines 61-65 are switched.
  2. There is a typo where a / (divide) is replaced by a second =.
    As a result, when use_median_duration is true, median duration is not used to calculate running speed. When it is False (else statement), the entire running speed array is accidentally set to the value of the median frame duration.

Addresses:

Addresses issue #1741

Type of Fix:

  • Bug fix (non-breaking change which fixes an issue)

Solution:

The solution is to switch the contents of the if/else statements and replace the
incorrect = by / (divide).

Checklist

  • My code follows
    Allen Institute Contribution Guidelines
  • My code is unit tested and does not decrease test coverage
  • I have performed a self review of my own code
  • My code is well-documented, and the docstrings conform to
    Numpy Standards
  • I have updated the documentation of the repository where
    appropriate
  • The header on my commit includes the issue number
  • My Pull Request has the latest AllenSDK release candidate branch
    rc/x.y.z as its merge target
  • My code passes all AllenSDK tests

Notes:

The code is not currently docstringed and the existing unit tests do not seem to test for actual running values extracted.

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #1747 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1747   +/-   ##
=======================================
  Coverage   36.16%   36.16%           
=======================================
  Files         346      346           
  Lines       33817    33817           
=======================================
  Hits        12229    12229           
  Misses      21588    21588           
Impacted Files Coverage Δ
...rain_observatory/extract_running_speed/__main__.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dd9dfe...9c07065. Read the comment docs.

@wbwakeman wbwakeman added the braintv relates to Insitute BrainTV program label Oct 13, 2020
@kschelonka
Copy link
Contributor

Thank you for your contribution @colleenjg. The code changes look good. Please remove the test.xml file from this commit, and please update the header on your commit to include the issue number this resolves. Thanks!

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.
@colleenjg colleenjg force-pushed the GH-1741/bugfix/running-speed-main branch from 727d63c to afdc06f Compare October 13, 2020 23:27
@colleenjg
Copy link
Contributor Author

Thank you for your contribution @colleenjg. The code changes look good. Please remove the test.xml file from this commit, and please update the header on your commit to include the issue number this resolves. Thanks!

Oh, right! Sorry about that. Done.

@kschelonka kschelonka self-requested a review October 13, 2020 23:48
@kschelonka kschelonka merged commit 18f49c7 into AllenInstitute:master Oct 15, 2020
@kschelonka
Copy link
Contributor

Thanks Colleen, we will update and push out a new patch build soon. For immediate use you can pip install from the master branch.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants