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

Fixing "GCXS matmul => slice leads to incorrect results" #611

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

EuGig
Copy link
Contributor

@EuGig EuGig commented Dec 4, 2023

It fixes #607. The solution simply inverts the order of indices and of data before returning them in _dot_csr_csr.
(I did not write any test, since I'm not implementing a new feature. Let me know if I have to do that.)

@hameerabbasi
Copy link
Collaborator

Could you perhaps add a regression test, I.e. a test that would trigger the bug before and now doesn't.

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Merging #611 (b8e1818) into master (7c0c962) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
+ Coverage   91.95%   91.97%   +0.02%     
==========================================
  Files          20       20              
  Lines        3330     3340      +10     
==========================================
+ Hits         3062     3072      +10     
  Misses        268      268              

@EuGig
Copy link
Contributor Author

EuGig commented Dec 4, 2023

Please ignore the last commit. I got an unexpected _utills.py.swp file. I will get rid of it. For the regression test I've thought of something like that:

Code

# tests for good slicing
assert np.array_equal(x[0].data, y[0])

# inverting indices and data to cause slicing error
x.indices = x.indices[::-1]
x.data = x.data[::-1]
assert not np.array_equal(x[0].data, y[0])

where x is the sparse version of a tensor product and y is the dense one.

hameerabbasi
hameerabbasi previously approved these changes Dec 5, 2023
@hameerabbasi
Copy link
Collaborator

It seems like a lot of existing tests are failing -- Would it be possible for you to look into that?

@hameerabbasi hameerabbasi dismissed their stale review December 5, 2023 10:46

Failing tests.

@EuGig
Copy link
Contributor Author

EuGig commented Dec 5, 2023

Yes, of course, already on that.

@EuGig
Copy link
Contributor Author

EuGig commented Dec 6, 2023

Hi @hameerabbasi, is it normal to have failing tests without any modification? I got 178 tests failed after simply cloning the repo.

@hameerabbasi
Copy link
Collaborator

That's quite odd -- I'll look into it in depth this weekend, but certainly not normal.

@hameerabbasi
Copy link
Collaborator

Hi @hameerabbasi, is it normal to have failing tests without any modification? I got 178 tests failed after simply cloning the repo.

Did you do an editable pip install before running the tests? If not, that's normal.

@EuGig
Copy link
Contributor Author

EuGig commented Dec 19, 2023

Oh yeah sorry, I fixed that. Now tests are passing.

@EuGig
Copy link
Contributor Author

EuGig commented Dec 19, 2023

Actually, pre-commit.ci is failing. I don't know why!

@hameerabbasi
Copy link
Collaborator

Actually, pre-commit.ci is failing. I don't know why!

That can be ignored for now.

@hameerabbasi hameerabbasi self-requested a review December 19, 2023 10:29
@hameerabbasi hameerabbasi merged commit 0e283ff into pydata:master Dec 19, 2023
12 of 13 checks passed
@hameerabbasi
Copy link
Collaborator

Thanks for the fix, @EuGig!

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.

GCXS matmul => slice leads to incorrect results
2 participants