Logging #131
Logging #131
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #131 +/- ##
========================================
- Coverage 93.2% 92.4% -0.9%
========================================
Files 45 46 +1
Lines 2400 2253 -147
========================================
- Hits 2239 2082 -157
- Misses 161 171 +10 ☔ View full report in Codecov by Sentry. |
| @@ -39,8 +39,8 @@ class DavidsonSymEigsSolver : public JDSymEigsBase<DavidsonSymEigsSolver<OpType> | |||
| Vector m_diagonal; | |||
|
|
|||
| public: | |||
| DavidsonSymEigsSolver(OpType& op, Index nev, Index nvec_init, Index nvec_max) : | |||
| JDSymEigsBase<DavidsonSymEigsSolver<OpType>, OpType>(op, nev, nvec_init, nvec_max) | |||
| DavidsonSymEigsSolver(OpType& op, Index nev, Index nvec_init, Index nvec_max, std::unique_ptr<LoggerBase<Scalar, Vector>> logger = nullptr) : | |||
There was a problem hiding this comment.
I do not prefer default arguments instead just have two constructors. The reason for it is that if we eve change the interface or add a second default argument it will all break down in user code.
There was a problem hiding this comment.
I had it implemented this way at first, but I wanted to reduce code duplication so I went with default args. I will change it back since I agree with your assessment.
include/Spectra/GenEigsBase.h
Outdated
| if (logger) | ||
| m_logger = std::move(logger); |
There was a problem hiding this comment.
I prefer brackets around one line if statements, but it is a nitpick
There was a problem hiding this comment.
and the rest of the codebase does not do it so. :)
There was a problem hiding this comment.
I prefer it as well but I will leave it since the code base doesn't do that
include/Spectra/GenEigsBase.h
Outdated
| if (m_logger) | ||
| m_logger->iteration_log(i, nconv, m_ncv, m_ritz_val.head(m_nev), m_resid, m_ritz_conv); |
There was a problem hiding this comment.
I think it would be nicer to have an iteration_data structure because if we want to log more things this function signature would have to change everywhere, of course then we would maybe have to take copies.
There was a problem hiding this comment.
Sure this makes sense. I will push some commits with this change. Even if we make copies, the size of the objects shouldn't be overly large so this will probably be ok.
There was a problem hiding this comment.
you can also take references in the constructor. So you could have a datastructure that just returns const references as well
include/Spectra/JDSymEigsBase.h
Outdated
| @@ -51,6 +54,7 @@ class JDSymEigsBase | |||
|
|
|||
| private: | |||
| CompInfo m_info = CompInfo::NotComputed; // status of the computation | |||
| std::unique_ptr<LoggerBase<Scalar, Vector>> m_logger; | |||
There was a problem hiding this comment.
thing is this makes the class non copyable, what do we do about that?
There was a problem hiding this comment.
We could use shared_ptr? There is no need for this to be unique_ptr
There was a problem hiding this comment.
the other option would be to just have a * and have the user of the library manage the lifetime of the logger
include/Spectra/LoggerBase.h
Outdated
| /// | ||
| /// Virtual logging function | ||
| /// | ||
| virtual void iteration_log(const Index& iteration, const Index& number_of_converged, const Index& subspace_size, const Vector& current_eigenvalues, const Vector& residues, const BoolArray& current_eig_converged); |
There was a problem hiding this comment.
as said I think we should add a Datastructure here, maybe even an abstract baseclass so we can use this also for the other solvers. @yixuan would this be worth extending to the other solvers?
|
Thanks for the quick review and the advice! |
|
@JensWehner If you could take another pass I'd appreciate it! |
include/Spectra/GenEigsBase.h
Outdated
|
|
||
| private: | ||
| BoolArray m_ritz_conv; // indicator of the convergence of Ritz values | ||
| CompInfo m_info; // status of the computation | ||
| std::shared_ptr<LoggerBase<Scalar, ComplexVector>> m_logger; |
There was a problem hiding this comment.
I am not sure if a shared_ptr is a good idea, because I copy would habe interesting problems then, because we would only have a shallow copy.
There was a problem hiding this comment.
Yeah sorry I wanted to try to use something safer than a raw pointer, but I realize now I would have to rewrite the copy constructor. That is still an option, but I switched to raw ptrs for now.
There was a problem hiding this comment.
Small nitpicks otherwise I like the addition, but let us see what @yixuan says.
…s. This commit can be rolled back if desired
This implements a virtual Logging class which can be implemented by the user to log each iteration. This implements the proposed implementation "A" on #103.
The added tests are very simple and demonstrate an example of the derived logging class.
Closes #103