-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add protection vs Minuit2 Fatal Root Error and update VxErrCorr in Vx3DHLTAnalyzer #38687
Conversation
@cmsbuild please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38687/30988
|
A new Pull Request was created by @francescobrivio for master. It involves the following packages:
@malbouis, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @jfernan2, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Gauss3D->Minimize(); | ||
try { | ||
Gauss3D->Minimize(); | ||
} catch (...) { |
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 this construct is forbidden, or at least is flagged as such by the S/A
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, rule 7.11
https://cms-sw.github.io/cms_coding_rules.html
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.
Following the coding rules I can try something like:
if (!Gauss3D->Minimize()) {
throw cms::Exception("FitFailed") << "Vx3DHLTAnalyzer \tInitial matrix not pos. def");
}
Would this be better?
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.
catching cms::Exception
or std exceptions is allowed. What is not allowed is using catch(...)
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.
addressed in 76fa9de
Hi guys, |
Ok cool. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bb6b2a/26146/summary.html Comparison SummarySummary:
|
@cmsbuild please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38687/31001
|
Pull request #38687 was updated. @malbouis, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @jfernan2, @ggovi, @francescobrivio, @micsucmed, @rvenditti can you please check and sign again. |
type bugfix |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bb6b2a/26171/summary.html Comparison SummarySummary:
|
+db
|
+1 |
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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
try { | ||
Gauss3D->Minimize(); | ||
} catch (cms::Exception& er) { | ||
edm::LogError("Vx3DHLTAnalyzer") << "\tCaught Minuit2 exception: " << er.what(); |
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.
given the problems recently observed (again) with the online DQM client (see log), after catching the exception and logging the error, shouldn't we return the function with an error state instead of carrying on?
I am not sure the rest of the computation make sense if the fit fails.
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.
@vicha-w is there a way to reproduce offline the failure reported in the data-taking mattermost channel , e.g. by making available the input streamer files that are giving rise to the issue.
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.
see #39285
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.
Hi @mmusich. Sorry I didn't reach out. I have given LS files that caused the error in beamspot clients to @francescobrivio this morning.
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.
Hi @vicha-w sorry there is some confusion here: the streamer files you gave me this morning are NOT related to this issue, they are for different studies.
PR description:
In recent Fills ( >=7920 ) the
beampixel
DQM client has been crashing with error:This PR:
VxErrCorr
parameter inVx3DHLTAnalyzer
from1.2
to1.0
, similarly to theerrorScale
of thebeam
client which has been recently fixed in Update errorScale for BeamSpot Legacy DQM client #38632.try/catch
protections are added inVx3DHLTAnalyzer.cc
in order to avoid crashes of the whole client in case of Fatal Errors coming from Minuit2 minimizationcout
s toedm:Log*
s inVx3DHLTAnalyzer
PR validation:
Code compiles.
Run privately on FEVT events and fit now converges, also, in case it fails, the Fatal Error is catched correctly by the LogError avoiding the crash of the
beampixel
client.If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Not a backport, but 12_4_X and 12_3_X backports will be provided soon
FYI @dinardo