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

BUG: Fix typecasting problem in scipy.sparse.lil_matrix truediv #19408

Merged
merged 5 commits into from Nov 8, 2023

Conversation

vetschn
Copy link
Contributor

@vetschn vetschn commented Oct 19, 2023

Reference issue

Closes #19403

What does this implement/fix?

This fixes the inconsistent behavior I observed when dividing a scipy.sparse.lil_matrix by a scalar of differing type. Now the resulting dtype is checked and adjusted if necessary.

@j-bowhay j-bowhay added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse labels Oct 19, 2023
scipy/sparse/_lil.py Outdated Show resolved Hide resolved
Co-authored-by: Dan Schult <dschult@colgate.edu>
Copy link
Member

@perimosocordiae perimosocordiae left a comment

Choose a reason for hiding this comment

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

A unit test covering this would be quite useful. It looks like we have something similar for DOK already (see TestDOK.test_dok_divide_scalar), so let's add something similar to the TestLIL class (next to test_scalar_mul).

@@ -287,6 +287,9 @@ def _mul_scalar(self, other):
def __truediv__(self, other): # self / other
if isscalarlike(other):
new = self.copy()
new_dtype = np.result_type(self.dtype, np.array(other).dtype)
if new_dtype != self.dtype:
new.data = new.data.astype(new_dtype)
Copy link
Member

Choose a reason for hiding this comment

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

This won't work. The data member of LIL-format objects is a numpy array of Python lists, with dtype=object.

To change the dtype, we need to do something like this:

for i, rowvals in enumerate(new.data):
    new.data[i] = rowvals.astype(new_dtype)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have noticed this. Isn't just the following what we really want?

new.dtype =  np.result_type(self, other)

The list data is actually modified correctly. Things only start breaking when trying to convert to a different storage format, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... sorry that my solution is actually quite wrong in a number of ways.

@vetschn is correct that the values in new.data are correct (complex values).
It is the dtype that is not updated. The values are actually python objects. So they morph to what they need to become to hold the correct value.

It is definitely true that we need a test. That will show us what will work. :)
I think @vetschn 's solution should work. I have tried it at an interactive interpreter.
But there are many subtleties with dtype that I haven't experienced. :)

Copy link
Member

@perimosocordiae perimosocordiae left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding the test!

@dschult
Copy link
Contributor

dschult commented Nov 5, 2023

Can someone restart the failing test? It looks like it timed out during the build phase of the test.

@tylerjereddy tylerjereddy added this to the 1.12.0 milestone Nov 8, 2023
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Nov 8, 2023
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

CI is clean now, core dev approval, and locally the full suite passes on this branch as well.

In it goes with backport label, thanks for your first contribution to SciPy.

@tylerjereddy tylerjereddy merged commit 6f16ecc into scipy:main Nov 8, 2023
21 of 22 checks passed
@tylerjereddy tylerjereddy modified the milestones: 1.12.0, 1.11.4 Nov 16, 2023
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Nov 16, 2023
…y#19408)

* Fix typecasting problem in lil_matrix truedriv

* Added test

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Nov 18, 2023
…y#19408)

* Fix typecasting problem in lil_matrix truedriv

* Added test

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: scipy.sparse.lil_matrix division by complex number leads to inconsistent behavior
5 participants