8000 BUG: sparse: Validate explicit shape if given with dense argument to coo_matrix by perimosocordiae · Pull Request #9920 · scipy/scipy · GitHub
[go: up one dir, main page]
Skip to content

BUG: sparse: Validate explicit shape if given with dense argument to coo_matrix #9920

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2019
Merged

BUG: sparse: Validate explicit shape if given with dense argument to coo_matrix #9920

merged 1 commit into from
Mar 11, 2019

Conversation

perimosocordiae
Copy link
Member

Fixes gh-9919.

There are a couple routes we could take when validating the shape kwarg in this situation, but given the context of the original bug, it should be sufficient to raise an error whenever the given shape (intentional or not) doesn't match the shape of the dense matrix being converted (the first argument).

Note that once scipy starts converting code to be explicitly py3k-only, we could consider marking the shape and dtype arguments as keyword only. This would prevent these sorts of errors more broadly, though perhaps at the expense of breaking existing user code.

@perimosocordiae perimosocordiae mentioned this pull request Mar 8, 2019
Closed
@rgommers
Copy link
Member
rgommers commented Mar 8, 2019

@mattip another random PyPy failure, haven't seen this before:

Traceback (most recent call last):
  File "setup.py", line 488, in <module>
    setup_package()
  File "setup.py", line 420, in setup_package
    import numpy
  File "/usr/local/site-packages/numpy/__init__.py", line 142, in <module>
    from . import core
  File "/usr/local/site-packages/numpy/core/__init__.py", line 40, in <module>
    from . import multiarray
  File "/usr/local/site-packages/numpy/core/multiarray.py", line 44, in <module>
    _reconstruct.__module__ = 'numpy.core.multiarray'
AttributeError: readonly attribute '__module__'
Exited with code 1

does that need a bug report somewhere, or is it a flake?

@rgommers rgommers added the maintenance Items related to regular maintenance tasks label Mar 8, 2019
@rgommers rgommers added this to the 1.3.0 milestone Mar 8, 2019
@rgommers
Copy link
Member
rgommers commented Mar 8, 2019

Note that once scipy starts converting code to be explicitly py3k-only, we could consider marking the shape and dtype arguments as keyword only. This would prevent these sorts of errors more broadly, though perhaps at the expense of breaking existing user code.

That would indeed break quite some code. There may be a couple of exceptions, but in general I think we can only use keyword-only arguments for new code.

@mattip
Copy link
Contributor
mattip commented Mar 9, 2019

This was fixed in #9840. Rebase the pr off master. It is using an old pypy version.

@rgommers
Copy link
Member
rgommers commented Mar 9, 2019 •
edited
Loading

This was fixed in #9840. Rebase the pr off master. It is using an old pypy version.

Ah right. The failure didn't look familiar and it was a fresh PR, so I didn't expect that. Sorry for the noise.

What we need to do is fix CircleCI if we can, this is getting really annoying. It should test the merge commit, not the commit from the PR itself.

EDIT: or better, move away from CircleCI, since we're using too many resources there anyway (gh-9111)

@rgommers
Copy link
Member
rgommers commented Mar 9, 2019

I tracked down the relevant CircleCI issue and explained the problem again: https://circleci.com/ideas/?idea=CCI-I-926. There's a voting feature, if people want to push the vote button that may be useful.

@rgommers
Copy link
Member
rgommers commented Mar 9, 2019

@perimosocordiae could you add the example from the issue as a regression test?

@perimosocordiae
Validate explicit shape if given with dense arg
eb58eaa 
With a motivating test case added.
@perimosocordiae
Copy link
Member Author

@rgommers Sure, tests added. I also rebased, so hopefully we'll see the pypy tests pass.

@rgommers rgommers merged commit 0d6506a into scipy:master Mar 11, 2019
@rgommers
Copy link
Member

All green, in it goes. Thanks @perimosocordiae

@perimosocordiae perimosocordiae deleted the coo-check-shape branch March 11, 2019 00:19
@jorisvandenbossche
Copy link
Contributor
jorisvandenbossche commented Mar 11, 2019 •
edited
Loading

It might be that this broke a test in pandas (a test constructing a sparse matrix started failing the last hours). The test basically does:

from scipy import sparse

row = [0, 3, 1, 0]
col = [0, 3, 1, 2]
data = [4, 5, 7, 9]
sp_array = sparse.coo_matrix(data, (row, col))

So using lists and not arrays to create a small dummy matrix. That worked before, but now started raising an error in check_shape (see eg https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=9238&view=logs&jobId=3a03f79d-0b41-5610-1aa4-b4a014d0bc70), so I am assuming it is related to this PR (didn't actually check, only looked at the commits the last hours to see which one is related).

It's only in the testing code, not in actual functionality, so not a big problem for pandas (and it is also easy to fix the testing code to use arrays). But, just wanted to signal this (not sure what guarantees scipy gives on coercing the input).

@jorisvandenbossche jorisvandenbossche mentioned this pull request Mar 11, 2019
Closed
@jorisvandenbossche
Copy link
Contributor

Looking into it further, this is actually a wrong usage on our side, which was exactly the purpose of this PR to catch such shape mistakes ;-)

We are passing the data, (row, col) not inside a tuple, so gets interpreted as the shape.

So ignore my message above.

@perimosocordiae
Copy link
Member Author

Glad to see the PR is already helping! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks scipy.sparse
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@perimosocordiae @rgommers @mattip @jorisvandenbossche
0