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

ENH: add PROPACK wrapper for improved sparse SVD #14433

Merged
merged 145 commits into from Sep 7, 2021
Merged

Conversation

mckib2
Copy link
Contributor

@mckib2 mckib2 commented Jul 18, 2021

Reference issue

Closes #857 (A Wrapper for PROPACK)

What does this implement/fix?

Additional information

  • To keep things simple, we decided not to expose any PROPACK-specific options yet
  • PROPACK doesn't return orthogonal singular vectors corresponding with zero singular values. This is basically the same issue as svds returning nans for zero input matrix #3452 reported (and fixed) for ARPACK. This should be fixed for PROPACK in a separate PR
  • Reviewers should think carefully about how we call _svdp from svds -- that is, how the keywords passed to svds map to PROPACK arguments. We've had to make some decisions about that and how to do the input validation
  • This PR supports old RandomStates as this is an old function and the old solvers have always been using RandomState. The behavior of svds should be the same before and after this PR

Known/discovered issues:

  • svds(solver='arpack') crashes when setting initial random vector with which='SM'
  • PROPACK itself seems to have trouble with extreme values of shifts when irl_mode=True. We chose a reasonable default value for shifts that seems to avoid this issue. shifts is not currently exposed as an option, so while it's worth noting this PROPACK issue somewhere, it shouldn't affect this PR.

mckib2 and others added 30 commits December 29, 2020 15:28
Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
BENCH: sparse.linalg: svds: add benchmarks
TST: sparse.linalg: propack: add test against example provided with PROPACK
ENH: handle conda compiler -fopenmp flags
@github-actions github-actions bot added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org gitpod scipy.stats labels Sep 6, 2021
@mdhaber
Copy link
Contributor

mdhaber commented Sep 6, 2021

I think we've resolved all the recent comments except this one - @mckib2 can you let us know?
And while you're at it, your opinion on this would be helpful.

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks Matt for the updates! I am approving provided the reply from @mckib2 on the pyf files (since we have some checked in, it should be fine though). Before I merge, could you merge master so we see the doc one last time? (seemingly we don't merge on CircleCI before building the doc)

(also do you prefer a squash or do you want to clean the branch? It's a lot of work so you might want to keep some commits. I would also be fine merging as is since it's large work. Let me know 😃)

scipy/sparse/linalg/eigen/tests/test_svds.py Outdated Show resolved Hide resolved
@mdhaber
Copy link
Contributor

mdhaber commented Sep 6, 2021

@mckib2 is squash merge OK? Since PROPACK is in a submodule, we don't need a commit here for just adding PROPACK, right?

@mckib2
Copy link
Contributor Author

mckib2 commented Sep 6, 2021

@mckib2 is squash merge OK? Since PROPACK is in a submodule, we don't need a commit here for just adding PROPACK, right?

Correct, no commit needed to separate out an "add PROPACK" step. A squash merge is preferable as I believe there are binaries hiding in the history from early development

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks Nicholas for the precisions.

Just fixing the doc (and a slight inconsistency between some parameters default value. We use optional in the rest so using it for k as well) and I will merge.

scipy/sparse/linalg/eigen/_svds_doc.py Outdated Show resolved Hide resolved
scipy/sparse/linalg/eigen/_svds_doc.py Outdated Show resolved Hide resolved
scipy/sparse/linalg/eigen/_svds_doc.py Show resolved Hide resolved
@tupui tupui merged commit 02bf2e7 into scipy:master Sep 7, 2021
@tupui
Copy link
Member

tupui commented Sep 7, 2021

Thanks @mckib2 and @mdhaber! Great project and solves a long standing item of the wish list. Can you update the list and the release notes?

@tupui tupui added this to the 1.8.0 milestone Sep 7, 2021
@rgommers
Copy link
Member

rgommers commented Sep 7, 2021

Very nice to have this merged, great work @mckib2 and @mdhaber!

CircleCI timed out on time_svds in the merge commit, and also on this PR I see. So that benchmark needs a tweak to run faster.

@mdhaber
Copy link
Contributor

mdhaber commented Sep 7, 2021

@rgommers I thought this was resolved, so I guess it is an intermittent issue : /
This is different from a normal timeout, right? asv forcibly kills benchmarks that are taking longer than 60s by default (and I think that's unchanged in SciPy), so it seems that asv loses control of this one?
Last time I checked, the problem does not occur locally, and all the benchmarks are pretty quick. Do you have any suggestions for debugging this? Maybe split the benchmarks up so we get some console output when each one finishes, trigger CI with [skip actions] [skip azp] to run only CircleCI, and repeat a few times until we narrow it down? Or just remove some benchmarks and hope for the best, without trying to track down the offending one(s)?

Update: no problem locally; every benchmark takes PROPACK less than a second on a 2013 MacBook Pro.

[100.00%] ··· ===== ========== ============ ============ =============
              --                                solver                
              ---------------- ---------------------------------------
                k    problem      arpack       lobpcg       propack   
              ===== ========== ============ ============ =============
                20    abb313    11.7±0.6ms   64.1±10ms     17.1±0.4ms 
                20   illc1033   43.5±10ms    77.4±10ms     26.0±0.2ms 
                20   illc1850   61.0±10ms     97.2±1ms     109±0.4ms  
                20    qh1484    14.5±0.1ms   138±0.4ms    18.1±0.05ms 
                20   rbs480a    37.9±0.3ms   127±0.9ms     34.3±0.1ms 
                20   tols4000    91.8±1ms     361±40ms     99.8±0.1ms 
                20   well1033   28.1±0.4ms   69.9±0.8ms   22.7±0.08ms 
                20   well1850   47.0±0.3ms    109±1ms      66.1±0.6ms 
                20   west0479   11.1±0.8ms     failed      8.99±0.6ms 
                20   west2021   24.9±0.2ms    175±1ms       24.6±1ms  
                50    abb313    40.9±0.2ms    13.9±2ms    30.8±0.05ms 
                50   illc1033    281±70ms     218±20ms     104±0.5ms  
                50   illc1850    131±1ms      301±10ms      248±2ms   
                50    qh1484    33.9±0.8ms     failed      45.4±0.3ms 
                50   rbs480a     77.7±1ms     326±20ms      95.2±3ms  
                50   tols4000    239±20ms    1.12±0.04s     162±4ms   
                50   well1033    350±20ms      failed      84.8±0.3ms 
                50   well1850    182±7ms      303±10ms      353±2ms   
                50   west0479    32.1±8ms     220±8ms      26.6±0.2ms 
                50   west2021   39.6±0.2ms    512±9ms      48.2±0.5ms 
               100    abb313    41.3±0.3ms   20.4±0.4ms    29.4±0.2ms 
               100   illc1033   82.8±0.4ms    55.0±9ms      162±10ms  
               100   illc1850    188±8ms      660±20ms      250±2ms   
               100    qh1484     122±6ms       failed       156±5ms   
               100   rbs480a     144±3ms      108±1ms       122±5ms   
               100   tols4000    694±20ms    3.00±0.02s     254±8ms   
               100   well1033   3.03±0.08s   44.4±0.7ms    138±0.9ms  
               100   well1850    269±20ms    758±100ms      361±3ms   
               100   west0479   58.6±0.5ms   64.1±0.1ms   66.2±0.09ms 
               100   west2021    126±1ms     1.48±0.05s     158±3ms   
              ===== ========== ============ ============ =============

@mdhaber
Copy link
Contributor

mdhaber commented Sep 7, 2021

Thanks @tupui @rgommers @mckib2 @jakevdp! Added to release notes.

@rgommers
Copy link
Member

rgommers commented Sep 9, 2021

every benchmark takes PROPACK less than a second on a 2013 MacBook Pro.

Hmm, not sure what's going on then. It's fairly consistently failing on CircleCI. There may be a build issue or something that causes it to hang on that config.

@rgommers
Copy link
Member

rgommers commented Sep 9, 2021

Update: no problem locally; every benchmark takes PROPACK less than a second on a 2013 MacBook Pro.

This is definitely not the case on my machine. I think you meant that timing results are a second or less. That's too much, timing results as reported by asv should typically be in the 100us - 10 ms range.

Just run $ time python runtests.py --bench --no-build time_svds to see how long it takes for you locally. For me it's over 2 minutes, and I have a fast machine. That benchmark needs breaking up, and using smaller input arrays.

rgommers added a commit to rgommers/scipy that referenced this pull request Sep 9, 2021
This is a follow-up to scipygh-14433

Note that a LOBPCG benchmark is failing, that should be addressed
separately.
@rgommers
Copy link
Member

rgommers commented Sep 9, 2021

Follow-up in gh-14708.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmarks Running, verifying or documenting benchmarks for SciPy Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org enhancement A new feature or improvement scipy.sparse.linalg scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A Wrapper for PROPACK (Trac #330)
4 participants