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

[FIX] Bincount: Fix crash on array with all nans #2831

Merged
merged 3 commits into from
Jan 7, 2018

Conversation

pavlin-policar
Copy link
Collaborator

Issue

Utility version of bincount crashed on sparse vector of all NaNs.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Dec 16, 2017

Codecov Report

Merging #2831 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2831      +/-   ##
==========================================
- Coverage   81.95%   81.72%   -0.23%     
==========================================
  Files         321      326       +5     
  Lines       54841    55703     +862     
==========================================
+ Hits        44946    45526     +580     
- Misses       9895    10177     +282

@lanzagar lanzagar added this to the 3.9 milestone Dec 18, 2017
Copy link
Contributor

@nikicc nikicc left a comment

Choose a reason for hiding this comment

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

I approve the code changes.

The test, however, doesn't really tests the change since it is missing implicit zeros. Looking at the codecov shows that the newly-added line is not covered.

@pavlin-policar
Copy link
Collaborator Author

Thanks! I've had to change the dense_sparse decorator so it would, in addition to the previous ways it ran tests, also run test for sparse array with no explicit zeros. Otherwise any test I wrote for bincount would be pointless, since it only crashed when the only value in the array was NaN.

@nikicc
Copy link
Contributor

nikicc commented Jan 7, 2018

@pavlin-policar looks good to me.

@nikicc nikicc merged commit acb7dd1 into biolab:master Jan 7, 2018
@pavlin-policar pavlin-policar deleted the util-bincount-allnans branch January 7, 2018 20:35
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.

4 participants