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

Adding check to ensure L1Menu consistency in the HLT : 112X #31461

Merged
merged 1 commit into from Sep 30, 2020

Conversation

Sam-Harper
Copy link
Contributor

@Sam-Harper Sam-Harper commented Sep 14, 2020

PR description:

The L1 result is packed into the RAW data as 512 bits which each bit corresponding to a given L1 seed. Which seed corresponds to a given bit is determined by the L1 menu which is sent as conditions data. It is possible for the wrong L1 menu to be read if the corresponding entry in the global tag is incorrect. In this case the bits will not correspond to the L1 seeds the HLT thinks they do and thus when it thinks its requiring EG40 to pass, its really requiring Mu16 (as a hypothetical example).

This PR adds the function which can convert the the firmware uuid to the hash of the firmware uuid send in the RAW data along with the L1 results. This can be then checked by the L1 GT emulator (L1TGlobalProducer) to ensure that the correct menu is being re-emulated and throw an exception if not.

Note L1TGlobalProducer has two distinct use cases

  1. to emulate the L1 Global trigger when emulating the L1 in the MC or emulating a different L1 menu on the data
  2. to create the map of L1 objects to seeds which is needed for the HLT to properly use the seeds in its filters

It is use case 2) where we need to check that the menu we are emulating to create the l1 object map is the same as the menu used to create the L1 result in the RAW data. This is why this is an optional check (which will always be enabled in the HLT) as it doesnt make sense for use case 1)

Zero physics changes are expected.

PR validation:

runTheMatrix tests succeeded.

testing with the incorrect menu resulted in the exception being thrown

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@Sam-Harper
Copy link
Contributor Author

@Martin-Grunewald: FYI

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31461/18366

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

CondFormats/L1TObjects
L1Trigger/L1TGlobal

@benkrikler, @christopheralanwest, @tocheng, @cmsbuild, @rekovic, @tlampen, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @mmusich, @seemasharmafnal, @tocheng this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 15, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: e58937d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c8351b/9339/summary.html
CMSSW: CMSSW_11_2_X_2020-09-15-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c8351b/9339/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2620306
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2620272
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@silviodonato
Copy link
Contributor

Any comments from @cms-sw/alca-l2 @cms-sw/db-l2 @cms-sw/l1-l2 ?

@tlampen
Copy link
Contributor

tlampen commented Sep 18, 2020

+alca

@Sam-Harper
Copy link
Contributor Author

Hi @cavana, thanks. You can also see https://hypernews.cern.ch/HyperNews/CMS/get/L1TriggerSW/869.html for more details. Its been bubbling along for a while (it got L1 TDRed and then covided leading to large delays)

@cavana
Copy link
Contributor

cavana commented Sep 30, 2020

Hi @Sam-Harper, many thanks for this development and glad to hear that you are back to healthy -- I'm not sure which is worse, being TDRed or covided :-). I've quickly reviewed the PR and it looks good to me -- I'm happy to endorse it. Thanks for your patience as I try to come up to speed with all things uGT emulator. Best wishes, Rick

@Sam-Harper
Copy link
Contributor Author

Thanks. And for covid, I meant, it fell by the wayside of general covid chaos and disruption (it was being discussed in late march) and was forgotten about, not that I got covid!

Thanks for endorsing it, this will greatly increase the robustness of the HLT

@silviodonato
Copy link
Contributor

please test
I will merge this PR in the evening IB
@rekovic I take @cavana's comment as a +1 from l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 30, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: e58937d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c8351b/9668/summary.html
CMSSW: CMSSW_11_2_X_2020-09-30-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c8351b/9668/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2542225
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2542202
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit d0c137b into cms-sw:master Sep 30, 2020
@rekovic
Copy link
Contributor

rekovic commented Oct 1, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2020

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 be automatically merged.

h ^= data[2] << 16;
case 2:
h ^= data[1] << 8;
case 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sam-Harper Is the purpose of case 3: and case 2: to fall through (i.e. is the break; omitted on purpose)?

(spotted by gcc 9 warnings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know who would know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, @rekovic might know though. Here we are emulating firmware deployed at p5 though. So even if it is "buggy" its still correct I think as long as it agrees with the hardware output at p5.

Copy link
Contributor

Choose a reason for hiding this comment

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

@makortel @Sam-Harper , looks like the fall through is on purpose. I have opened a PR #31828 to silence GCC 9 warnings

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

10 participants