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

Add Unpacker for L1T Calo Layer 1 to unpack CICADA #45037

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

aloeliger
Copy link
Contributor

PR description:

This PR adds an unpacker to the L1T Calo Layer 1 Unpacking Setup to explicitly unpack CICADA (found in FED 1356, AMC slot 7). This PR relies on data-types introduced in it's companion PR #44222. #44222 Unpacks the uGT received CICADA scores, while this PR unpacks the Calo sent CICADA scores.

PR validation:

All code compiles and has had code formatting applied. This Unpacker has also been tested against events with the CaloSummary/CICADA cards in DAQ. A screenshot of some debugging output of the development of the unpacker is attached. On the left side you can see the unpacked data from FED 1356, which matches the dumped FED output seen on the right (CICADA/CaloSummary highlighted)

image

Also shown is a demonstration that the correct CICADA bits are received in the correct order, and the resulting constructed score, matched to the expected hex number (when transformed into a 16 bit fixed-point number, with 8 integer bits, and 8 bits after the decimal point, to within machine epsilon, or display precision).

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:

This PR is not a backport, and is not expected to need a backport. It is however, a feature of an upcoming data-taking tool, so it is not impossible that a backport will be requested for it.

@eyigitba @slaurila FYI, this is one of the DPG requested pieces for CICADA that we have made for our own commissioning purposes now. Packers will be made when we are satisfied with commissioning progress using these unpackers.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 24, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45037/40336

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • EventFilter/L1TRawToDigi (l1)

@epalencia, @cmsbuild, @aloeliger can you please review it and eventually sign? Thanks.
@eyigitba, @thomreis, @Martin-Grunewald, @missirol, @dinyar this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@aloeliger
Copy link
Contributor Author

test parameters:
pull_requests = #44222

@aloeliger
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a1eda2/39510/summary.html
COMMIT: 077e16d
CMSSW: CMSSW_14_1_X_2024-05-23-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45037/39510/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 2 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 16 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3338862
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3338833
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45037/40671

  • This PR adds an extra 24KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

Pull request #45037 was updated. @cmsbuild, @aloeliger, @epalencia can you please check and sign again.

@aloeliger
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a1eda2/40014/summary.html
COMMIT: bf7e838
CMSSW: CMSSW_14_1_X_2024-06-20-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45037/40014/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 5 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3345018
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3344995
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@aloeliger
Copy link
Contributor Author

+l1

  • uGT unpacker is now ready

@cmsbuild
Copy link
Contributor

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. @sextonkennedy, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: #44222

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit c0b1550 into cms-sw:master Jun 25, 2024
12 checks passed
@mmusich
Copy link
Contributor

mmusich commented Jun 26, 2024

Congratulations. Merging this PR alone completely obliterated the IB CMSSW_14_1_X_2024-06-25-2300, https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/ib/CMSSW_14_1_X

@fwyzard
Copy link
Contributor

fwyzard commented Jun 26, 2024

I guess that's because:

Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: #44222

which is not yet ready to be merged ?

Comment on lines +29 to +50
std::vector<uint32_t> cicadaWords = {0, 0, 0, 0};
//the last 4 words are CICADA words
for (uint32_t i = 2; i < 6; ++i) {
cicadaWords.at(i - 2) = ((block.payload().at(i)) >> 28);
}

float cicadaScore = convertCICADABitsToFloat(cicadaWords);
res->push_back(0, cicadaScore);
return true;
}
}

//convert the 4 CICADA bits/words into a proper number
float CICADAUnpacker::convertCICADABitsToFloat(const std::vector<uint32_t>& cicadaBits) {
uint32_t tempResult = 0;
tempResult |= cicadaBits.at(0) << 12;
tempResult |= cicadaBits.at(1) << 8;
tempResult |= cicadaBits.at(2) << 4;
tempResult |= cicadaBits.at(3);
float result = 0.0;
result = (float)tempResult * pow(2.0, -8);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be replaced by a simpler implementation, see https://github.com/cms-sw/cmssw/pull/44222/files#r1653052470 .

In fact, it would be better to share the duplicate code between EventFilter/L1TRawToDigi/plugins/implementations_stage2/CICADAUnpacker.cc and EventFilter/L1TRawToDigi/plugins/implementations_stage2/CaloSummaryUnpacker.cc.

Copy link
Contributor Author

@aloeliger aloeliger Jun 26, 2024

Choose a reason for hiding this comment

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

I am working on this this morning, however just for posterity's sake, I will note that the exact bits grabbed are ever so slightly different, being the first 4 bits of the last 4 words for FED 1356, instead of the first 4 bits of the first 4 words for FEDs 1404 & 1405.

In any case, you are entirely correct that this could be simplified down from the ad-hoc first drafts. I can do that for the uGT one while the HLT experts review. I am not sure what happens for this one now.

@smuzaffar
Copy link
Contributor

@cms-sw/orp-l2 , we might not be able to converge on #44222 before 11h00 IB. I would suggest to revert this PR for 11h00 ? Any objections @cms-sw/l1-l2 ?

@smuzaffar
Copy link
Contributor

I have opened #45308 ( which is revert of this PR)

@aloeliger
Copy link
Contributor Author

No objections here. I can sign the revert quickly.

My apologies for this, I thought this was tested and tagged with the uGT unpacker as external.

@mmusich
Copy link
Contributor

mmusich commented Jun 26, 2024

My apologies for this, I thought this was tested and tagged with the uGT unpacker as external.

I think everything was done correctly on your side, but the requires-external flag was neglected when merging as pointed out at #45037 (comment) (see cms-sw/cms-bot#2147 for a discussion of this general problem).

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