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] Statistics.unique: Fix Sparse Return Order For Negative Numbers #2572

Merged
merged 2 commits into from
Oct 21, 2017

Conversation

pavlin-policar
Copy link
Collaborator

@pavlin-policar pavlin-policar commented Sep 9, 2017

Issue
  • The statistics module implementation of unique returned incorrectly ordered arrays in case of a negative number e.g.
>>> unique(csr_matrix([-1, 0, 0, 1, 2]))
array([ 0, -1,  1,  2], dtype=int64)

>>> np.unique([-1, 0, 0, 1, 2])
array([-1,  0,  1,  2])
  • The statistics module implementation of nanunique did not support any parameters that unique supported.
Description of changes

Fixes above issues. This PR depends on #2698 for the dense_sparse testing utility.

Includes
  • Code changes
  • Tests
  • Documentation

@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch 6 times, most recently from 11eee67 to 3a2f4f8 Compare September 9, 2017 19:25
@codecov-io
Copy link

codecov-io commented Sep 9, 2017

Codecov Report

Merging #2572 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2572      +/-   ##
==========================================
- Coverage   75.83%   75.83%   -0.01%     
==========================================
  Files         338      338              
  Lines       59545    59545              
==========================================
- Hits        45156    45154       -2     
- Misses      14389    14391       +2

@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch 5 times, most recently from 792b230 to 08238b1 Compare September 12, 2017 10:44
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 only checked a0dceaf and it looks OK to me.

Other commits are a part of #2558, right?

@pavlin-policar
Copy link
Collaborator Author

pavlin-policar commented Sep 13, 2017

Yes, this PR contains only 2 commits:

  • a0dceaf fixes negative number positions
  • 8add499 passes *args, **kwargs from nanunique to the unique function call.

(I've also squashed the last commit - pylint fixes into the last one for a cleaner history. No actual code changes were made)

@nikicc
Copy link
Contributor

nikicc commented Sep 15, 2017

OK, then when #2558 is merged this just needs a rebase and IMO it's ready to merge.

@nikicc nikicc self-assigned this Oct 18, 2017
@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch 2 times, most recently from 447fd36 to bc688c8 Compare October 20, 2017 11:30
@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch 3 times, most recently from 326d512 to 28ceda0 Compare October 20, 2017 13:46
@nikicc
Copy link
Contributor

nikicc commented Oct 21, 2017

Please rebase this as #2698 has been merged.

@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch from 28ceda0 to c5a3368 Compare October 21, 2017 16:17
@pavlin-policar pavlin-policar force-pushed the statistics-utils-unique branch from c5a3368 to 3e08ba0 Compare October 21, 2017 16:21
@pavlin-policar
Copy link
Collaborator Author

@nikicc Rebased.

@nikicc nikicc changed the title [FIX] Statistics.unique: Incorrect sparse return order [FIX] Statistics.unique: Fix Sparse Return Order For Negative Numbers Oct 21, 2017
@nikicc nikicc merged commit 63df7e1 into biolab:master Oct 21, 2017
@pavlin-policar pavlin-policar deleted the statistics-utils-unique branch October 22, 2017 07:45
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.

3 participants