Skip to content
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

Protection against missing discriminators for offline taus in L1 DQM #35305

Merged
merged 2 commits into from Sep 17, 2021

Conversation

lathomas
Copy link
Contributor

PR description:

This (hopefully) is a short term bug fix of the issue described in #35304
In very rare cases, the anti electron discriminators seem to be missing for offline tau, leading to a crash in the L1 DQM code.
A protection is added to skip such offline taus.

PR validation:

Checked that the crash in #35304 is gone after this fix.

@tvami

@tvami
Copy link
Contributor

tvami commented Sep 16, 2021

type bug-fix

@tvami
Copy link
Contributor

tvami commented Sep 16, 2021

urgent

@@ -704,6 +704,19 @@ void L1TTauOffline::getProbeTaus(const edm::Event& iEvent,
TLorentzVector mytau;
mytau.SetPtEtaPhiE(tauIt->pt(), tauIt->eta(), tauIt->phi(), tauIt->energy());

if ((*antimu)[tauCandidate].workingPoints.empty()) {
LogWarning("This offline tau has no antimu discriminator, skipping");
Copy link
Contributor

@tvami tvami Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lathomas isnt the structure with the LogWarning something like
edm::LogWarning("category") << "message to be printed";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think this should be
edm::LogWarning("L1TTauOffline") << "This offline tau has no antimu discriminator, skipping";
Please do the same with the other ones too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, will do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lathomas please dont forget to push your fix :)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35305/25320

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lathomas for master.

It involves the following packages:

  • DQMOffline/L1Trigger (dqm, l1)

@emanueleusai, @ahmad3213, @cmsbuild, @rekovic, @jfernan2, @pmandrik, @cecilecaillol, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@rociovilar this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Sep 16, 2021

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35305/25331

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

Pull request #35305 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @rekovic, @jfernan2, @pmandrik, @cecilecaillol, @pbo0, @rvenditti can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Sep 16, 2021

@cmsbuild , please test

@lathomas
Copy link
Contributor Author

@tvami

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ccde59/18676/summary.html
COMMIT: d9cf860
CMSSW: CMSSW_12_1_X_2021-09-16-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35305/18676/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000833
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3000805
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Sep 16, 2021

All looks good, @jfernan2 @rekovic please sign this as soon as possible, so that we can submit the backport and cut a new release for CRAFT, thanks!

@jfernan2
Copy link
Contributor

+1

@tvami
Copy link
Contributor

tvami commented Sep 17, 2021

Hi @perrotta @qliphy give the urgency and the simplicity of this PR, would you please consider to merge this without the L1 signature?

@qliphy
Copy link
Contributor

qliphy commented Sep 17, 2021

+1

@qliphy
Copy link
Contributor

qliphy commented Sep 17, 2021

merge

@lathomas
Copy link
Contributor Author

@tvami I won't be able to take care of the backport(s). Can we identify someone else?

@tvami
Copy link
Contributor

tvami commented Sep 17, 2021

@tvami I won't be able to take care of the backport(s). Can we identify someone else?

I did it myself, please see #35322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants