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

[Tests] Nose -> pytest #14

Open
leycec opened this issue Jun 10, 2021 · 7 comments
Open

[Tests] Nose -> pytest #14

leycec opened this issue Jun 10, 2021 · 7 comments

Comments

@leycec
Copy link

leycec commented Jun 10, 2021

sparse_dot_mkl is phenomenal, dramatically improving both runtime and solver accuracy (which was very surprising to us) for our use case. Because sparse_dot_mkl is phenomenal, it'd be great if the test suite could be migrated from Nose to pytest.

Sadly, Nose is officially dead:

Nose has been in maintenance mode for the past several years and will likely cease without a new person/team to take over maintainership.

But you're in luck! Pytest is a well-maintained drop-in replacement for Nose. Having just packaged sparse_dot_mkl for Gentoo Linux, I can confirm that all tests pass without modification under pytest. This means that migrating from Nose to pytest is as trivial as replacing nose with pytest in both setup.py and .travis.yml.

Thanks again for the fantastic binding, @asistradition. It can't be overstated how much of an improvement this is over the default SciPy implementation. 😮

@asistradition
Copy link
Collaborator

Sure, that's no problem - it's only on nose because of inertia. I'll switch it the next time I push an update.

@leycec
Copy link
Author

leycec commented Jun 11, 2021

Super! Nose is slated to be removed from most Linux distros soon, which complicates packaging the current version of sparse_dot_mkl a little.

Thanks again. The Flatiron Institute continues to shine on us all.

@asistradition
Copy link
Collaborator

No problem. Im finally getting around to the wrapper for the iterative sparse solvers; I'll switch to pytest on the next release with that.

@leycec
Copy link
Author

leycec commented Jun 11, 2021

Im finally getting around to the wrapper for the iterative sparse solvers...

...that sounds amazing. Incredibly excited about all of this! If I'm reading the MKL docos correctly:

  • The current sparse_qr_solve_mkl() routine wraps the direct sparse solver based on PARDISO referred to as Intel MKL PARDISO, which helps explain why sparse_qr_solve_mkl() is so friggin' amazing.
  • Hypothetical new routines debuting in some upcoming release will wrap the iterative MKL sparse solvers, namely:
    • Both forms of the RCI conjugate gradient (CG) solver, namely:
      • For system of equations with a single right-hand side.
      • For systems of equations with multiple right-hand sides.
    • The RCI flexible generalized minimal residual (FGMRES) solver.

Is that right? If so, it might be useful to explicitly note in the README.md that sparse_qr_solve_mkl() is actually Intel MKL PARDISO. I guarantee GitHub stars will immediately leap into the stratosphere, because Intel-MKL-PARDISO-on-Python is a thing everyone really wants needs. Grant funding will surely follow shortly thereafter. 😉

It'll also be great fun to play around with all of these alternate solvers. CG and GFMRES seem a bit less flexible than QR, but probably make up for that transgression by being more space- and/or time-performant. Fun!

Even more interestingly, all of this pushes sparse_dot_mkl closer to being a drop-in replacement for scipy.sparse.linalg. This is beginning to exceed the bounds of this specific issue, but do you think it might be feasible to make a new sparse_dot_mkl.scipy submodule (or something, something) mimicking the scipy.sparse.linalg API?

Advantages of a distinct sparse_dot_mkl.scipy namespace might include:

  • Simplifying migration from scipy.sparse.linalg to sparse_dot_mkl, because we want everyone to use the latter and not the former.
  • Preserving the existing sparse_dot_mkl API for backward compatibility, because we don't want to break anything.

Very well. I'll stop now. Thanks yet again for the tremendous everything. You do the Good Work™ here.

@asistradition
Copy link
Collaborator

sparse_qr_solve_mkl isn't PARDISO - it's a separate QR factorization and solver. I don't think there's any reason PARDISO wouldn't work from python though and shouldn't be terribly difficult to implement.

The CG and FGMRES solvers are in a dev branch (dd0741a) - they still need a bit of work and a good test package but I should be able to finish them soon. (Mostly I want FGMRES but I did CG cause it wasn't much extra work)

I'll see what I can do about an alternative API that's a drop-in for scipy.sparse.linalg; I can at least replicate most of the functionality of spsolve, cg and gmres (eigs and svd should be easy too). There's a few things that I probably can't do (I don't know what the API is for the LinearOperator).

@leycec
Copy link
Author

leycec commented Jun 12, 2021

sparse_qr_solve_mkl isn't PARDISO - it's a separate QR factorization and solver.

Curious and curiouser. So Intel now maintains two divergent MKL-based sparse solvers:

  • Intel MKL PARDISO based on the Cholesky decomposition.
  • Intel MKL Sparse QR based on the QR decomposition.

That's... interesting! Intel gonna Intel. I wonder what the practical tradeoffs (if any) between the two solvers are? From the little that I know, the QR decomposition is considerably more computationally expensive to compute than the Cholesky decomposition – but sparse_dot_mkl is blazing fast, which means that I know nothing.

they still need a bit of work and a good test package but I should be able to finish them soon.

👍 👍 👍

I can at least replicate most of the functionality of spsolve, cg and gmres (eigs and svd should be easy too).

That would be impressive beyond words. The scipy.sparse.linalg API is rather well-designed on the whole; it's just the implementation that leaves something okay, a lot of things to be desired.

Excruciatingly hyped for all of this.

@asistradition
Copy link
Collaborator

Changed from nose to pytest in 86228d0 and released it as v0.7.2.

I'll work on the other stuff and try to get that out soon-ish, so I'm going to leave this issue open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants