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
DM-13669 log meas and ref outliers and notebook to plot them #77
Conversation
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 good. Couple of minor comments.
src/FitterBase.cc
Outdated
@@ -171,7 +172,8 @@ MinimizeResult FitterBase::minimize(std::string const &whatToFit, double nSigmaC | |||
MeasuredStarList msOutliers; | |||
FittedStarList fsOutliers; | |||
int nOutliers = findOutliers(nSigmaCut, msOutliers, fsOutliers); | |||
totalOutliers += nOutliers; | |||
totalMeasOutliers += msOutliers.size(); | |||
totalRefOutliers += fsOutliers.size(); |
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.
Is it still useful to have nOutliers
in addition to msOutliers
and fsOutliers
?
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, because it's the number of outliers in that step only (saves doing msOutliers.size() + fsOutliers.size()
twice). I'll make a comment to that effect.
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.
Not sure a comment is necessary, it just occurred to me as a possible side effect of the change.
src/FitterBase.cc
Outdated
if (nSigmaCut != 0) { | ||
LOGLS_INFO(_log, "Total number of outliers (Measured + Reference = Total): " | ||
<< totalMeasOutliers << " + " << totalRefOutliers << " = " | ||
<< totalMeasOutliers + totalRefOutliers); |
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 wording "Total of X + Y = Total" seems a bit weird. Is the initial "Total" still meaningful?
5c5942e
to
858c1f3
Compare
858c1f3
to
76ecb3f
Compare
No description provided.