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

dotDivide changes to denseMatrix when the inputs are sparseMatrix #3219

Closed
dvd101x opened this issue Jun 14, 2024 · 6 comments · Fixed by #3307
Closed

dotDivide changes to denseMatrix when the inputs are sparseMatrix #3219

dvd101x opened this issue Jun 14, 2024 · 6 comments · Fixed by #3307

Comments

@dvd101x
Copy link
Collaborator

dvd101x commented Jun 14, 2024

Hi,

I found an odd behavior of sparse matrices in dotDivide on 13.0.0

math.dotDivide([1, 2], [1, 1])
// [1, 2]
// OK

math.dotDivide(math.sparse([1,2]), 1)
// SparseMatrix [1, 2]
// OK

math.dotDivide(math.sparse([1, 2]), math.sparse([1, 1]))
// DenseMatrix [[1], [2]]
// it's odd that the shape changes and it returns a DenseMatrix

math.dotDivide(math.sparse([1, 2]), math.sparse([1]))
// TypeError: undefined is not an object (evaluating 's[t]')

The implementation of matrixAlgorithmSuite in dotDivide seems different than the one in dotMultiply maybe it has something to do with the issue.

I found this while working on #3196, found that for sparse matrices broadcasting isn't working very well and went into a rabbit hole to find this separate issue.

@josdejong
Copy link
Owner

Thanks for reporting! We should indeed align this and make sure the output is a SparseMatrix for sparse inputs. I think we should mark it as a breaking change, since the current behavior may be depended upon.

@nkumawat34
Copy link
Contributor

can i work on this issue.

@nkumawat34
Copy link
Contributor

nkumawat34 commented Oct 30, 2024

I found that in dotMultiply method algorithm which is included doing sparseMatrix dotMultiply sparseMatrix give sparseMatrix
Algorithm is MatAlgo09xS0Sf
dotMultiply

In dotDivide method ,algorithm which is included doing sparseMatrix dotDivide sparseMatrix gives DenseMatrix
Algorithm is MatAlgo07xSSf
dotDivide

@josdejong josdejong mentioned this issue Oct 31, 2024
@josdejong
Copy link
Owner

Thanks Neeraj.

I see this issue was picked up recently via #3287 by @rana-aakash so let's await a response there.

@rana-aakash
Copy link

Hello @josdejong
Thanks for the reminder , I have submitted PR related to this at #3307

@josdejong
Copy link
Owner

Fixed in v14.0.0, thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants