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

Update muon shower data formats for 2023 running #40890

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

dinyar
Copy link
Contributor

@dinyar dinyar commented Feb 27, 2023

PR description:

This is an update for the muon shower data formats (RegionalMuonShower and MuonShower) containing the changes that I think are needed for 2023 running. @elfontan and @eyigitba, could you please check whether this is all you'll need also on your side (i.e. uGT and EMTF, respectively).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40890/34373

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/L1TMuon (l1)
  • DataFormats/L1Trigger (l1)

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

@cmsbuild please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7633d6/30934/summary.html
COMMIT: c5423aa
CMSSW: CMSSW_13_1_X_2023-02-27-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40890/30934/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7633d6/30934/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7633d6/30934/git-merge-result

Build

I found compilation error when building:

>> Building LCG reflex dict from header file src/DataFormats/L1Trigger/src/classes.h
>> Compiling LCG dictionary: tmp/el8_amd64_gcc11/src/DataFormats/L1Trigger/src/DataFormatsL1Trigger/a/DataFormatsL1Trigger_xr.cc
>> Building shared library tmp/el8_amd64_gcc11/src/DataFormats/L1Trigger/src/DataFormatsL1Trigger/libDataFormatsL1Trigger.so
Copying tmp/el8_amd64_gcc11/src/DataFormats/L1Trigger/src/DataFormatsL1Trigger/libDataFormatsL1Trigger.so to productstore area:
>> Checking EDM Class Version for src/DataFormats/L1Trigger/src/classes_def.xml in libDataFormatsL1Trigger.so
error: class 'l1extra::L1ParticleMap' has a different checksum for ClassVersion 12. Increment ClassVersion to 13 and assign it to checksum 3029280877
Suggestion: You can run 'scram build updateclassversion' to generate src/DataFormats/L1Trigger/src/classes_def.xml.generated with updated ClassVersion
gmake: *** [tmp/el8_amd64_gcc11/src/DataFormats/L1Trigger/src/DataFormatsL1Trigger/libDataFormatsL1Trigger.so] Error 1
Leaving library rule at DataFormats/L1Trigger
>> Leaving Package DataFormats/L1Trigger
>> Package DataFormats/L1Trigger built


@elfontan
Copy link
Contributor

elfontan commented Feb 28, 2023

Thanks for this PR @dinyar!
Given that for this case we need an update of the DataFormats in anycase, does it make sense to remove completely the case mus0/mus1 (setMus0/setMus1) that are exactly the same as isOneNominalInTime/isOneTightInTime (setOneNominalInTime/setOneTightInTime), to avoid confusion in the future?
See here.
My feeling is that they are just leftover from the first implementation. Please let me know if I am wrong.

On the GT side, as soon as we receive an update from the uGT firmare side (about the update of utm/TME), we can proceed with the inclusion of this additional bit check including your code and testing everything together.

@dinyar
Copy link
Contributor Author

dinyar commented Feb 28, 2023

Given that for this case we need an update of the DataFormats in anycase, does it make sense to remove completely the case mus0/mus1 (setMus0/setMus1) that are exactly the same as isOneNominalInTime/isOneTightInTime (setOneNominalInTime/setOneTightInTime), to avoid confusion in the future?
See here.
My feeling is that they are just leftover from the first implementation. Please let me know if I am wrong.

Hi @elfontan,

Yes, I fully agree with the removal of those. I'll do so in the next update.

Cheers,
Dinyar

@dinyar
Copy link
Contributor Author

dinyar commented Mar 8, 2023

Hi @elfontan, @eyigitba,

I think in our private communication you both indicated that this PR was fine, but I'm not absolutely sure anymore. If you can quickly confirm here then I'll move this out of draft stage.

Cheers,
Dinyar

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40890/34511

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2023

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

@elfontan
Copy link
Contributor

elfontan commented Mar 8, 2023

Thank you @dinyar, I am fine with it!
I actually got lost about the meaning of oneLooseInTime, I had in mind the plan to trigger on twoLooseInTime only, but we can surely follow up offline.
On a similar note: are you sure that we want to remove entirely the descriptive part in L1Trigger/interface/MuonShower.h?
I found it extremely useful in the past months...

Cheers,
--Elisa

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40890/34544

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

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

Copy link
Contributor

@aloeliger aloeliger left a comment

Choose a reason for hiding this comment

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

@dinyar, I think it looks fine to me. If @elfontan or @eyigitba are fine with it, I think we're okay. When you're ready go ahead and change from draft to ready to merge, and I'll test it.

@eyigitba
Copy link
Contributor

@dinyar @aloeliger , this looks good to me. On the EMTF side we will implement the logic for assigning the isOneLooseInTime_ flag. I think that's all we need. Thanks!

@dinyar dinyar marked this pull request as ready for review March 13, 2023 08:56
@aloeliger
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7633d6/31236/summary.html
COMMIT: b67de57
CMSSW: CMSSW_13_1_X_2023-03-12-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40890/31236/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3550756
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3550731
  • 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

@dinyar & @eyigitba We may have some progress with updating the utm soon. Does this PR need to wait until the new utm version is available for any reason? Otherwise I am content to sign on it now.

@elfontan
Copy link
Contributor

elfontan commented Mar 13, 2023

Hi @aloeliger,
no, this PR is unrelated to the update of the utm libraries on the HMT side, they are relevant only for the update of the uGT emulator.
Cheers,
--Elisa

@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

+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.

6 participants