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
Change Minuit logger for CMS MessageLogger #32440
Change Minuit logger for CMS MessageLogger #32440
Conversation
e1b2312
to
311fc57
Compare
code-checks |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32440/20330
|
please test |
A new Pull Request was created by @mrodozov (Mircho Rodozov) for master. It involves the following packages: RecoLocalCalo/HcalRecAlgos @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -169,16 +170,16 @@ namespace PSFitter { | |||
|
|||
if (step <= 0) { | |||
std::string txtmsg = "Parameter " + name + " has zero or invalid step size - consider it as constant "; | |||
MN_INFO_MSG2("HybridMinimizer::SetVariable", txtmsg); | |||
edm::LogInfo("HybridMinimizer::SetVariable") << txtmsg << 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.
The std::endl
us unnecessary as MessageLogger adds the newline (unless intention is to have two newlines),
edm::LogInfo("HybridMinimizer::SetVariable") << txtmsg << std::endl; | |
edm::LogInfo("HybridMinimizer::SetVariable") << txtmsg; |
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.
Actually, would it be worth to avoid the intermediate string?
edm::LogInfo("HybridMinimizer::SetVariable") << "Parameter " << name << " has zero or invalid step size - consider it as constant";
or
edm::LogInfo("HybridMinimizer::SetVariable").format("Parameter {} has zero or invalid step size - consider it as constant", name);
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.
when the test is finished I'll add it, finished tests will keep the root rpm and avoid rebuilding it.
please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32440/20332
|
please test |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32440/20338
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32440/20465
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
c60fa05
to
508c123
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32440/20467
|
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e8085b/11769/summary.html Comparison SummarySummary:
|
+1
issue #32525 was added to keep track of possibly more appropriate treatment of the HybridMinimizer |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
See #32416
This makes use of LogInfo to substitute Minuit logger which was updated
One concern is what happens with the
MnPrint::SetLevel(PrintLevel())
calls now commented since the old Minuit was globally setting the level and now MnPrint::SetLevel requires an object
From what I checked the LogInfo is used already within the same package:
https://github.com/cms-sw/cmssw/blob/master/RecoLocalCalo/HcalRecAlgos/src/HcalSeverityLevelComputer.cc#L292
nn the same manner.
PR validation:
it builds with latest ROOT master changes and it should work without MnPrint at all