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

Bug fix on L1TrackJetEmulator #39479

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

gkaratha
Copy link
Contributor

PR description:

Bug fix on the L1 phase2 trackjet emulation.

Description of the problem: In cases were no tracks are found the code does not return empty containers. It returns nothing with result of failing in later steps or in offline analysis when we try to access them. This is important for samples with PU=0 were the possibility to have events w/o tracks is non-negligible.
Solution: Straightforward just added the empty containers. The change is transparent, just the minimum to prevent the crashes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39479/32212

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • L1Trigger/L1TTrackMatch (upgrade, l1)

@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @beaucero, @trtomei this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f97616/27734/summary.html
COMMIT: ec12b28
CMSSW: CMSSW_12_6_X_2022-09-21-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39479/27734/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3624368
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3624344
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@cecilecaillol
Copy link
Contributor

+l1

@AdrianoDee
Copy link
Contributor

please test
(old tests, once successful, it's ready to go)

@@ -243,6 +244,10 @@ void L1TrackJetEmulationProducer::produce(Event &iEvent, const EventSetup &iSetu
delete[] mzb.clusters;
} else if (L1TrkPtrs_.empty()) {
Copy link
Contributor

@srimanob srimanob Oct 3, 2022

Choose a reason for hiding this comment

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

Is it not just else of this line,
if (!L1TrkPtrs_.empty()) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, yes it is just the else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change to else then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think is necessary yes. I am re-writting the code anyway. In the new version I have this [*]. Instead of adding multiple brackets with else and if I only check if it is enmpty and if so return empty pointers.

if (L1TrkPtrs_.empty()){
if (displaced_)
iEvent.put(std::move(L1TrackJetContainer), "L1TrackJetsExtended");
else
iEvent.put(std::move(L1TrackJetContainer), "L1TrackJets");
return;
}

Copy link
Contributor

@srimanob srimanob Oct 3, 2022

Choose a reason for hiding this comment

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

Sorry, I confuse. Will you rewrite the follow up PR? Then I am ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,

I am re-writing the code altogether. I can make this change in this PR or I can leave it as is for now, and then when l1t offline gets merged it will change automatically. Sorry for the confusion.

Best,
george

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thanks. Then I think we can leave this for now, when PR test is done and success, we can sign.

@AdrianoDee
Copy link
Contributor

AdrianoDee commented Oct 4, 2022

Did the test get stuck?
(@smuzaffar)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 4, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f97616/27936/summary.html
COMMIT: ec12b28
CMSSW: CMSSW_12_6_X_2022-10-03-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39479/27936/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-f97616/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3432650
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3432625
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 49 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@AdrianoDee
Copy link
Contributor

+upgrade

  • putting an empty collection in the event preventing subsequent failures.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 4, 2022

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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

+1

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

ping bot

@cmsbuild cmsbuild merged commit 97723d2 into cms-sw:master Oct 4, 2022
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.

6 participants