-
Notifications
You must be signed in to change notification settings - Fork 6
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
breaking: Less stinky Cov Osc #273
Conversation
Hi @KSkwarczynski, thank you for contributing to MaCh3! Please wait for MaCh3 developers to review your PR. If no one answers within a week, please message people from this list: https://github.com/orgs/mach3-software/teams/mach3admin . While waiting, please enjoy this Use this action on your projects. Use jokes on issues instead. |
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.
Very minor comments but looks good
@@ -32,7 +32,6 @@ class covarianceBase { | |||
/// @brief Destructor | |||
virtual ~covarianceBase(); | |||
|
|||
// Setters |
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.
A truly next level nit-pick, but could do a \defgroup Setters here to group the setters and keep the doxygen a little bit tidier https://www.doxygen.nl/manual/grouping.html
covariance/covarianceBase.h
Outdated
@@ -103,7 +106,7 @@ class covarianceBase { | |||
void RandomConfiguration(); | |||
|
|||
/// @brief Check if parameters were proposed outside physical boundary | |||
virtual int CheckBounds(); | |||
int CheckBounds(); |
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.
could make this function const?
covariance/covarianceBase.h
Outdated
void printNominal(); | ||
/// @brief Print prior, current and proposed value for each parameter | ||
void printNominalCurrProp(); | ||
void printPars(); | ||
/// @warning only for backward compatibility | ||
/// @todo remove it | ||
void printPars() {printNominalCurrProp();}; |
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.
These seem const-able too
Pull request description
Plenty of tidy to cov classes
Changes or fixes
Examples