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

TST: fix sparse constructor test with large memory footprint #19184

Merged
merged 3 commits into from Sep 7, 2023

Conversation

dschult
Copy link
Contributor

@dschult dschult commented Sep 5, 2023

Fix test in test_construct that broke from #18862 because of change in private function.

This wasn't seen by that PR because it doesn't run on a 32-bit machine.
Thanks @WarrenWeckesser !!

@@ -539,7 +539,7 @@ def test_concatenate_int32_overflow(self):
n = 33000
A = csr_array(np.ones((n, n), dtype=bool))
B = A.copy()
C = construct._compressed_sparse_stack((A,B), 0)
C = construct._compressed_sparse_stack((A,B), 0, False)
Copy link
Member

Choose a reason for hiding this comment

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

Shoudn't it be True to maintain the existing behavior of the test?

Also, as long as we're touching this code, let's make it nicer...

Suggested change
C = construct._compressed_sparse_stack((A,B), 0, False)
C = construct._compressed_sparse_stack((A, B), axis=0,
return_spmatrix=True)

(assuming you agree that it should be True).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #18862 we changed the test to use csr_array so I think this should be False here.
And -- yes, we should make it prettier.
I've been aiming for 88 chars or less on a line (usually less). is there a tradition in scipy for what to aim for?

Copy link
Member

Choose a reason for hiding this comment

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

SciPy's policy is still PEP 8, which says 79 characters max. There has been no explicit policy change that I know of, so that is what we should shoot for.

The degree to which our CI checks this has changed over the years, and we are now using fewer automated checks than we used to. Some time ago, ruff replaced pycodestyle. ruff does not check a lot of basic whitespace issues--ruff expects that black is used for that, but we have not adopted black, so our automated style checking ended up weaker than it used to be. ruff does check line length, but its default max is 88 characters. Allowing lines that long seems be an accident of the change to ruff, and not an explicit decision by SciPy devs to increase the recommended line length. I'm sure a lot of devs are OK with it, and I'm trying to be less pedantic 😃, but I still think we should shoot for 79 characters, and only exceed it in exceptional cases (and this does not look like an exceptional case; an extra line break is no big deal).

Copy link
Member

Choose a reason for hiding this comment

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

In #18862 we changed the test to use csr_array so I think this should be False here.

OK, I'll take your word for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazingly, when I run python dev.py test -v -j 32 -- -k "test_concatenate_int32_overflow" -rsx on current main, I get:

SKIPPED [1] scipy/_lib/tests/test_array_api.py:12: Array API test; set environment variable SCIPY_ARRAY_API=1 to run it. I'll shift that over to gh-19190.

Using the other incantation over there, I do indeed get the test passing here and failing on main.

@rgommers rgommers changed the title fix test with large memory footprint TST: fix sparse constructor test with large memory footprint Sep 6, 2023
@rgommers rgommers added this to the 1.12.0 milestone Sep 6, 2023
@tylerjereddy
Copy link
Contributor

I checked the test at two points in time re the C object type change:

  • on maintenance/1.11.x:
type(C):
 <class 'scipy.sparse._csr.csr_matrix'>
C.shape:
 (66000, 33000)
C.getnnz():
 2178000000
  • on the PR branch:
type(C):
 <class 'scipy.sparse._csr.csr_array'>
C.shape:
 (66000, 33000)
C.nnz:
 2178000000

The change to csr_array is described as intentional above, from the other listed PR that was reviewed by CJ/sparse folks, so I think we're good to go with this test repair now. I'll probably squash-merge with the two small linting commits.

@tylerjereddy tylerjereddy merged commit 709f0d3 into scipy:main Sep 7, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants