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

Add SparseRegularInverse::info() #111

Closed
Spammed opened this issue Jan 10, 2021 · 3 comments
Closed

Add SparseRegularInverse::info() #111

Spammed opened this issue Jan 10, 2021 · 3 comments

Comments

@Spammed
Copy link

Spammed commented Jan 10, 2021

Thank you for your implementation, I use it with great success and satisfaction.

I use SparseRegularInverse as an alternative to SparseCholesky via a template parameter, so I like it when both have the same interface.

I was missing SparseRegularInverse::info(), but it was easy to retrofit.

Noticing the current activity on the code so I thought maybe I should bring this little thing up. Because unfortunately lagging a own github repository set up, the patch is just copied here.

---
 include/Spectra/MatOp/SparseRegularInverse.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/Spectra/MatOp/SparseRegularInverse.h b/include/Spectra/MatOp/SparseRegularInverse.h
index 3abd0c1..dcfaec5 100644
--- a/include/Spectra/MatOp/SparseRegularInverse.h
+++ b/include/Spectra/MatOp/SparseRegularInverse.h
@@ -39,6 +39,7 @@ private:
     ConstGenericSparseMatrix m_mat;
     const int m_n;
     Eigen::ConjugateGradient<SparseMatrix> m_cg;
+	int m_info;  // status of the decomposition
 
 public:
     ///
@@ -55,6 +56,9 @@ public:
             throw std::invalid_argument("SparseRegularInverse: matrix must be square");
 
         m_cg.compute(mat);
+		m_info = (m_cg.info() == Eigen::Success) ?
+			SUCCESSFUL :
+			NUMERICAL_ISSUE;
     }
 
     ///
@@ -66,6 +70,12 @@ public:
     ///
     Index cols() const { return m_n; }
 
+	///
+	/// Returns the status of the computation.
+	/// The full list of enumeration values can be found in \ref Enumerations.
+	///
+	int info() const { return m_info; }
+
     ///
     /// Perform the solving operation \f$y=B^{-1}x\f$.
     ///
-- 
2.27.0.windows.1

@yixuan
Copy link
Owner

yixuan commented Jan 10, 2021

Hi @Spammed, thanks for the suggestion and the patch. In fact I had thought about including the info() function, but didn't do that due to the following reason. Eigen::ConjugateGradient is an iterative linear solver, whose compute() member function doesn't do any actual decomposition, so m_cg.info() is almost always "success" after calling compute(). The real computation and convergence test happen in the solve() member function.

But I get your point that adding an info() function makes the template programming easier, so I have made this change, by adding both the info() member function and the convergence test in solve().

Note that I worked in the 1.y.z branch, which will be the next major release. It breaks a lot of APIs, so you may need to change your code accordingly. Also see the discussion here.

@Spammed
Copy link
Author

Spammed commented Jan 10, 2021

Thanks for considering the idea and even greater thanks for the continued work on Spectra.

I am a friend of API refactoring in general and I am excited about it.
Let's see how much 'trouble' it will cause me to be able to use the improvements in the new version.

@yixuan
Copy link
Owner

yixuan commented Jan 10, 2021

Great! The test file should give you some ideas about how to change the code.

@Spammed Spammed closed this as completed Jan 13, 2021
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