-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Move iterative eigensolvers to the stdlib #24714
Conversation
efc27ab
to
e61914f
Compare
@@ -127,9 +127,6 @@ function choosetests(choices = []) | |||
"linalg/generic", "linalg/uniformscaling", "linalg/lq", | |||
"linalg/hessenberg", "linalg/rowvector", "linalg/conjarray", | |||
"linalg/blas"] | |||
if Base.USE_GPL_LIBS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing to me. ARPACK is not GPL-licensed, so why did we only test the Arnoldi stuff if we were using GPL libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing: maybe because it depends on SuiteSparse which is partly GPL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense. I guess I should modify the stdlib tests then to only run Arnoldi when we're okay with using GPL libraries? (i.e. when Base.USE_GPL_LIBS
is true
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ARPACK as such doesn't depend on anything in SuiteSparse but the wrappers use factorizations and when the input is sparse, which is the case in some of the tests, then the wrappers use sparse factorizations from SuiteSparse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the shift and invert solvers need SuiteSparse.
So the way to think of We should consider a more recognizable name than |
Yes, I also think that |
We should also avoid a name that sounds like it is about iterators. Maybe EigSubspace? |
A better name than |
IterativeEigenSolvers seems fine, though it's quite similar to JuliaMath/IterativeSolvers (not sure whether that's good or bad). According to Andreas, we do wrap most of ARPACK, but I suppose I would expect a package with a name like ARPACK to be a lower level wrapper over the library rather than providing higher level APIs as we do in this package. I'm pretty indifferent toward what this is called, so once a consensus is reached I'll just move forward with the agreed upon name. |
Likewise, i.e. I would expect the package |
I thought that eventually, the ARPACK wrappers would move to ARPACK.jl, and the rest of the higher level functionality would move to IterativeSolvers.jl. |
That also seems like a good option. |
Let's go ahead with |
Sounds great. Thanks all for the feedback. |
d40d009
to
af67190
Compare
CI failures are unrelated (32-bit Windows couldn't connect, macOS couldn't load OpenBLAS). Unless there are any further comments, I plan to merge this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks @ararslan
! :)
This PR moves
eigs
andsvds
into an Arnoldi package in the stdlib. This was very self-contained and was a pretty easy move. Julia's binary dependency on ARPACK is now only for this package, which can eventually be moved out entirely.thereby eliminating a GPL-licensed binary dependency.(Just kidding, ARPACK is 3-clause BSD.)There might be a couple of uses ofDone.eigs
that I missed, hence WIP.