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 "one loose" showers from EMTF and "two loose showers in different sectors" from uGMT to L1Tntuples #41233

Merged
merged 3 commits into from
Mar 31, 2023

Conversation

dinyar
Copy link
Contributor

@dinyar dinyar commented Mar 30, 2023

PR description:

This PR adds the newly added "one loose" showers that are produced at the EMTF level and the "two loose in different sectors" produced in the uGMT to the L1TNtuples.

(mostly for @elfontan, but this might also be useful for @eyigitba)

PR validation:

Ran with command cmsDriver.py l1Ntuple -s RAW2DIGI --python_filename=data_def.py -n 10000 --no_output --era=Run3 --data --conditions=124X_dataRun3_Prompt_v4 --customise=L1Trigger/Configuration/customiseReEmul.L1TReEmulFromRAW --customise=L1Trigger/L1TNtuples/customiseL1Ntuple.L1NtupleRAWEMU --filein=/store/data/Commissioning2023/Cosmics/RAW/v2/000/365/300/00000/0bee10f0-6e0c-4efd-9ab5-c58993f0b709.root --fileout=HMT_Cosmics_L1Ntuple.root (provided by @elfontan, thanks for that!). Looking at the resulting NTuple I saw one loose shower from EMTF in every event (this was somewhat surprising to me, but the code is quite straightforward and I'm not sure how loose the thresholds are.. ), I saw two loose showers in different sectors for about 42% of events.

@dinyar
Copy link
Contributor Author

dinyar commented Mar 30, 2023

One more question: should this also be backported to 13_0_X? I had suspected that it doesn't need to be, but let me know if it would be useful.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41233/34949

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dinyar (Dinyar Rabady) for master.

It involves the following packages:

  • L1Trigger/L1TMuon (l1)
  • L1Trigger/L1TNtuples (l1)

@epalencia, @cmsbuild, @cecilecaillol, @aloeliger can you please review it and eventually sign? Thanks.
@kreczko, @JanFSchulte, @eyigitba, @Martin-Grunewald, @missirol, @thomreis 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

@aloeliger
Copy link
Contributor

please test

@aloeliger
Copy link
Contributor

@dinyar This isn't emulation/something that is going to be needed for running at P5/T0 is it? If not I think we can forgo the back port to 13_0?

@dinyar
Copy link
Contributor Author

dinyar commented Mar 30, 2023

@aloeliger yes, indeed this only affects the L1TNtuples, nothing for P5. My only worry was that e.g. @elfontan or someone else might try to produce L1TNtuples with 13_0_X and miss the new quantities.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c939ff/31694/summary.html
COMMIT: 7eef0aa
CMSSW: CMSSW_13_1_X_2023-03-30-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41233/31694/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 9 lines from the logs
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3554236
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3554207
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@aloeliger
Copy link
Contributor

aloeliger commented Mar 30, 2023

@elfontan, Efe is out of touch for a while to my understanding. Would you like to state a preference on whether you would like a backport before I sign off?

@elfontan
Copy link
Contributor

Hey @dinyar @aloeliger,
thanks for this!
I agree that it is probably not strictly needed, but my personal preference would to have a coherent status of the code and changes between 130X and 131X, if possible, to avoid any confusion or misunderstanding in the future...

Cheers,
--Elisa

@aloeliger
Copy link
Contributor

aloeliger commented Mar 31, 2023

Okay, understood. We can check with ORP if they would be okay on a back-port of these changes, but I would guess that the preference is only those things strictly necessary in data taking operations are back-ported.

@aloeliger
Copy link
Contributor

+l1

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

@perrotta
Copy link
Contributor

perrotta commented Mar 31, 2023

Okay, understood. We can check with ORP if they would be okay on a back-port of these changes, but I would guess that the preference is only those things strictly necessary in data taking operations are back-ported.

Since this only touches the L1TNtuples, and the update is also not too complicated, I don't think there are counterindications in backporting it in the data taking release. Provided it can really help L1T operations and/or analyses somehow, of course.

@perrotta
Copy link
Contributor

+1

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.

5 participants