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

Unhelpful error message when calling scipy.sparse.save_npz on a dok_matrix #7718 #8049

Merged
merged 4 commits into from
Oct 19, 2017
Merged

Unhelpful error message when calling scipy.sparse.save_npz on a dok_matrix #7718 #8049

merged 4 commits into from
Oct 19, 2017

Conversation

RothNRK
Copy link
Contributor

@RothNRK RothNRK commented Oct 17, 2017

#7718

The comments in the issue were implemented and now a NotImplementedError: Save is not implemented for sparse matrix of format {}. error is raised.

@@ -72,7 +69,9 @@ def save_npz(file, matrix, compressed=True):
arrays_dict.update(row=matrix.row, col=matrix.col)
else:
raise NotImplementedError('Save is not implemented for sparse matrix of format {}.'.format(matrix.format))

arrays_dict = dict(format=matrix.format.encode('ascii'),
Copy link
Member

Choose a reason for hiding this comment

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

This should update arrays_dict, not overwrite it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

@perimosocordiae perimosocordiae added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse labels Oct 18, 2017
@perimosocordiae
Copy link
Member

Thanks for the fix! Can you also add a test to scipy/sparse/tests/test_matrix_io.py that tries to call save_npz on a DOK matrix and assert_raises that we get a NotImplementedError?

…tempt to save an unsupoorted type is made.
x[0,1] = 1
save_npz('x.npz', x)

assert_raises(NotImplementedError, save_dok)
Copy link
Member

Choose a reason for hiding this comment

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

Can you have the assert_raises just check save_npz? That is:

x = dok_matrix(...)
...
assert_raises(NotImplementedError, save_npz, 'x.npz', x)

@@ -7,11 +7,11 @@

import pytest
from pytest import raises as assert_raises
from numpy.testing import assert_equal, assert_
from numpy.testing import assert_equal, assert_, assert_raises
Copy link
Member

Choose a reason for hiding this comment

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

This assert_raises isn't needed, as we're getting it from the import on the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change but can you please explain the advantage of implementing it as you suggested?

Copy link
Member

Choose a reason for hiding this comment

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

We're moving from nose to pytest. The version from numpy.testing is from nose, whereas the one imported on line 9 is from pytest. They do the same thing, but since we no longer require users to install nose to run the tests, we can't rely on numpy.testing.assert_raises being available.

@perimosocordiae perimosocordiae merged commit 8a994ca into scipy:master Oct 19, 2017
@perimosocordiae
Copy link
Member

Merged. Thanks again, @RothNRK !

@person142 person142 added this to the 1.1.0 milestone Oct 22, 2017
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.

None yet

3 participants