-
Notifications
You must be signed in to change notification settings - Fork 1.4k
No verbose teststat printouts #21212
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
base: master
Are you sure you want to change the base?
No verbose teststat printouts #21212
Conversation
Test Results 22 files 22 suites 3d 21h 38m 33s ⏱️ Results for commit 2627139. |
| break; | ||
| } else if (tries < maxtries) { | ||
| std::cout << " ----> Doing a re-scan first" << std::endl; | ||
| // std::cout << " ----> Doing a re-scan first" << std::endl; |
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.
@guitargeek can we use R__LOG_DEBUG to silence these by default but still have them available if one sets the debug level appropriately?
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've just committed a change to this effect, I hope I used it correctly.
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.
That's not how R__LOG_DEBUG is used: see core/foundation/inc/ROOT/RLogger.hxx for the definition and e.g. core/foundation/test/testLogger.cxx for the usage.
However I don't see it used anywhere in roofit so maybe rootfit uses something different, that's why I asked @guitargeek.
Also, as a general rule please always test that your code at the very least compiles before pushing, otherwise the CI can waste a lot of CPU time for something that's easily caught locally...
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.
Ah ok, sorry! I'm currently working on several PRs at the same time locally and didn't find it very easy to go back to this one to test (I did the edit via the webinterface), but you are correct, of course!
I'll roll back my change.
| throw std::runtime_error("likelihood node has no data attached!"); | ||
| } | ||
|
|
||
| std::vector<std::string> nllDistNames = valsToStringVec((*nllNode)["distributions"]); |
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.
Should this also be:
| std::vector<std::string> nllDistNames = valsToStringVec((*nllNode)["distributions"]); | |
| std::vector<std::string> nllDistNames = valsToStringVec(analysisNode["distributions"]); |
?
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.
No, this is correct as-is. Distributions are stored as part of the NLL, wheras the external constraints are stored as part of the analysis node (=ModelConfig).
But thanks for checking!
This Pull request:
remove some very verbose printouts from the ProfileLikelihoodTestStat
I guess the printouts could also be changed to use one of the RooFit printout mechanisms, but as it stands, they cannot be silenced, which is an issue since the test can be invoked from the ToyMCSampler inside the toy loop, which can easily generate O(10000) lines of output.
Checklist:
This PR fixes #