-
Notifications
You must be signed in to change notification settings - Fork 126
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
Logging #131
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer brackets around one line if statements, but it is a nitpick
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.
and the rest of the codebase does not do it so. :)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thing is this makes the class non copyable, what do we do about that?
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.
We could use shared_ptr? There is no need for this to be unique_ptr
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Thanks for implementing it, what a nice PR. I added some comments which I think can make it even more long term stable. Would be nice for the other solvers to adapt sth. similiar.
Great work!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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