-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 inconsistent shape of matrix returned by scipy2scipy_clipped
#2066
base: develop
Are you sure you want to change the base?
Conversation
hi @psorianom, can you please add tests for this function (at least a case that you described in issue)? |
hey @menshikh-iv , thanks for checking this out. Sure, I will do it as soon as possible. |
scipy2scipy_clipped
The entire function looks strange. Its documentation doesn't match the code comments:
@menshikh-iv do we have good unit tests here? The code looks dodgy. |
@piskvorky we have only one test for this function |
Hi all, sorry for the delay. Regarding the comments, yes, it is per row, so if the input is an array of n rows, the output will have n rows. This differs from All in all, I realize that the bug I created is not actually a bug per se, i.e., the code does what it should do according to its description (return the 'topn' elements of the greatest magnitude) without regards to the original size of the matrix. In my use case, I do need the original size of the matrix, as I use it later for other calculations. So maybe adding a flag to
|
@psorianom hello, sorry for delay
maybe, sounds compatible 👍 |
If so, it needs a clear description + motivation. I still find the docstrings confusing (and contradictory to code), and adding extra parameters won't help. |
Fixed inconsistent shape of matrix returned by scipy2scipy_clipped, as described in issue #2065
Solved by explicitly setting the second dimension of the new matrix to that of the input matrix.
Fixes #2065.