Skip to content
This repository was archived by the owner on Jan 10, 2026. It is now read-only.

FIX: remove heavy-weight example files - #2

Merged
mdhaber merged 1 commit into
masterfrom
remove-examples
Dec 14, 2021
Merged

mdhaber merged 1 commit into
masterfrom
remove-examples

Conversation

@mckib2

@mckib2 mckib2 commented Dec 13, 2021 •

Copy link
Copy Markdown
Contributor

I have not evaluated the changes this will have to the scipy.sparse.linalg.svds test suite, I suspect there will need to be some adjustments. This PR is only to suggest one proposed approach for dealing with the PROPACK-size problem

@mckib2
mckib2 requested review from mdhaber and rgommers December 13, 2021 23:27
@mdhaber

mdhaber commented Dec 13, 2021 •

Copy link
Copy Markdown
Contributor

This is fine if it's what we need to do. Yes, we'd need to change /scipy/sparse/linalg/tests/test_propack::test_examples to load the data from a new NPZ or something. @rgommers please confirm and we'll pull the trigger.

@mckib2

mckib2 commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

A couple other options (just to try to be complete and thoughtful):

  • create a "release" branch and a "testing" branch. The "release" branch would exclude large example files and be used by the source distribution and wheel providers. The "testing" branch would be checked out in our CI and could include all the large files necessary
  • store large files somewhere else and download them for testing only (might require someone to donate server space for us)

I'm also good with simply removing per this PR and then when updating the commit hash for PROPACK in SciPy-proper, add required data for the test @mdhaber pointed out above

@rgommers

Copy link
Copy Markdown
Member

+1 for just removing, and only keeping .npz files if needed for testing. These examples will never be modified anymore upstream, and anyway keeping numerical data in ascii files isn't a useful thing to do.

@mdhaber

mdhaber commented Dec 14, 2021 •

Copy link
Copy Markdown
Contributor

What size do you want the .npz files to be?

  • Converted to npz format, all the files are 37 MB.
  • Storing everything in the same file and eliminating some redundancy, we can store the same data in half that space.
  • Currently, we are storing both example matrices and a reference SVD provided with PROPACK. If we need to save more space, we can store the example matrices in only 587 KB and compare against a reference SVD computed with linalg.svd.
  • We could just generate random matrices and store nothing.

@rgommers

Copy link
Copy Markdown
Member
  • Converted to npz format, all the files are 37 MB.

That much still? I thought I had in a comment on the original PR that it'd be 2 MB. That would be acceptable (perhaps, just), I think 37 MB is way too much.

  • If we need to save more space, we can store the example matrices in only 587 KB and compare against a reference SVD computed with linalg.svd.

That sounds reasonable. I'm happy with either this approach or just using random matrices - whichever you prefer.

@mdhaber

mdhaber commented Dec 14, 2021 •

Copy link
Copy Markdown
Contributor

I see at the top of this PR is that the PROPACK submodule would be 2MB after removing all the example files.
But I don't think the full examples can be compressed that much automatically. The output includes many dense matrices, each with 256k elements.
I'll go the 587 KB route.

@mckib2

mckib2 commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

That much still? I thought I had in a comment on the original PR that it'd be 2 MB. That would be acceptable (perhaps, just), I think 37 MB is way too much.

@mdhaber was correct -- 2 MB without any examples or npz files

Looks like this is a good solution -- remove the files from here and add a reduced npz file to main scipy repo. I'd feel weird about creating a PR and then completing it myself, so I invite either of you @rgommers @mdhaber to complete if you're happy

@mdhaber

mdhaber commented Dec 14, 2021 •

Copy link
Copy Markdown
Contributor

I will, but I think we should merge scipy/scipy#15214 first so that there is no lapse in tests working

@mdhaber

mdhaber commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Oh, sounds like we need to update a commit hash, so there's no lapse even if we merge this first?

@mckib2

mckib2 commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

Oh, sounds like we need to update a commit hash, so there's no lapse even if we merge this first?

Correct, we need to explicitly commit a new hash in the parent SciPy repo for any changes here to take effect

@mdhaber
mdhaber merged commit e7b342c into master Dec 14, 2021
@mckib2
mckib2 deleted the remove-examples branch December 15, 2021 00:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants