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 additional track variables to the Run 3 scouting electron collection for low pT electrons. #41025

Merged
merged 1 commit into from Mar 17, 2023

Conversation

arsahasransu
Copy link
Contributor

@arsahasransu arsahasransu commented Mar 10, 2023

PR description:

-- The PR adds four variables to the Run3 scouting electron collection - pt, eta, phi and chi2/ndof.
-- This is necessary for low-energy electrons in the scouting collection.
-- The dataset format of the scouting output files should be compatible with previous releases.

PR validation:

-- Code validated with privately produced J/PsiToEE MC. Full presentation validated at the scouting meeting Scouting Meeting.
-- Basic code-check and code-formatting done.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41025/34552

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @arsahasransu (A.R.Sahasransu) for master.

It involves the following packages:

  • DataFormats/Scouting (core)
  • HLTrigger/Egamma (hlt)

@Martin-Grunewald, @Dr15Jones, @smuzaffar, @makortel, @cmsbuild, @missirol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato, @rovere 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

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

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

Please remove "Commit to" from the title of the PR.

HLTrigger/Egamma/plugins/HLTScoutingEgammaProducer.cc Outdated Show resolved Hide resolved
@arsahasransu arsahasransu changed the title Commit to add additional track variables to the Run 3 scouting electron collection for low pT electrons. Add additional track variables to the Run 3 scouting electron collection for low pT electrons. Mar 10, 2023
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41025/34559

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

Pull request #41025 was updated. @Martin-Grunewald, @Dr15Jones, @smuzaffar, @makortel, @cmsbuild, @missirol can you please check and sign again.

@missirol
Copy link
Contributor

please test

@makortel
Copy link
Contributor

Is this PR planned to be backported to 13_0_X?

@cmsbuild
Copy link
Contributor

+1

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

Comparison Summary

Summary:

  • You potentially removed 21 lines from the logs
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3530381
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3530353
  • 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

@missirol
Copy link
Contributor

Is this PR planned to be backported to 13_0_X?

I assume that's the idea.

@arsahasransu , could you please clarify? Also, are there any (other) DataFormat changes for Scouting targeting 13_0_X?

@makortel , for what I understand, this change is backward-compatible (it would still be possible to read Run3ScoutingElectron collections of existing ROOT files). Is that correct? In general, what are the other factors to keep in mind before backporting (or, considering to backport) this kind of DataFormat update? (esp. since Scouting is part of RAW, iiuc)

@makortel
Copy link
Contributor

for what I understand, this change is backward-compatible (it would still be possible to read Run3ScoutingElectron collections of existing ROOT files). Is that correct?

Correct. Adding data members is backwards compatible, and has been done in the past with Run 2 scouting data formats (between data taking years).

In general, what are the other factors to keep in mind before backporting (or, considering to backport) this kind of DataFormat update? (esp. since Scouting is part of RAW, iiuc)

Scouting is indeed part of RAW, and is constrained by the same backwards-compatibility guarantees. I think it is clearest to all parties if the updates (including the backports) are done sufficiently before any scouting data will be collected for this year. (technically updates could be done also later, but I'd be very careful with those)

From the technical standpoint

  • CMSSW is able to read any ROOT file written with the earlier format, the added data members will just be initialized as in the default constructor
  • the data format change breaks reading earlier streamer files (that contain affected objects)
    • streamer file compatibility is anyway guaranteed only for the exactly the same CMSSW X_Y_Z version (plus _patchN builds); but it is useful to be aware of it e.g. for Tier0 replays

@arsahasransu
Copy link
Contributor Author

@missirol @makortel We want the new scouting data to be collected with this new class. I am not sure that implies a backport is required because I am not aware of the version of CMSSW that is going online. However, if this is required, I will create a backport ASAP.
As far as backwards compatibility is concerned, I sort of assumed that adding new variables with an appropriate constructor and initialization without changing any earlier variables should be backwards compatible. I am not able to understand the streamer file compatibility argument completely. However, if you direct me to some material and suggest me some checks, I will produce the results fast.

@cmsbuild
Copy link
Contributor

Pull request #41025 was updated. @Martin-Grunewald, @Dr15Jones, @smuzaffar, @makortel, @cmsbuild, @missirol can you please check and sign again.

@makortel
Copy link
Contributor

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

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

Comparison Summary

Summary:

  • You potentially removed 14 lines from the logs
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3550819
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3550789
  • 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

@missirol
Copy link
Contributor

+hlt

  • backward-compatible extension of the Run3ScoutingElectron data format for 2023 data-taking (addition of GSF-Tracks-related variables)

    • increase in event size checked by Scouting group, and considered negligible, see #41025 (comment)
    • backward-compatibility tested on existing RAW files with the test described in #41040 (comment)
    • vars {d0,dz,charge} are replaced by vectors {trkd0,trkdz,trkcharge}
  • scouting producer HLTScoutingEgammaProducer updated accordingly

@makortel
Copy link
Contributor

@missirol

  • backward-compatibility tested on existing RAW files with the test described in #41040 (comment)

Do I understand correctly that you are working to get that test to be run as part of unit tests?

@missirol
Copy link
Contributor

@makortel

Do I understand correctly that you are working to get that test to be run as part of unit tests?

Yes, but the details of the test are not finalized.

The test in #41040 (comment) is basically done, but I need more time if I need to change it according to #41040 (comment) (i don’t necessarily disagree with the suggestion, but i haven’t worked out the details yet).

Since this PR implies a new 13_0_X release, it would be good not to delay it too much (so I tried to validate it ‘manually’).

Of course, DataFormats/Scouting is under Core-sw, so feel free to suggest how to proceed.

@makortel
Copy link
Contributor

@missirol Given the time constraint I'll rely on your manual validation, under the assumption you'd be able to submit a PR adding the test in the near future. Thanks!

@makortel
Copy link
Contributor

+core

@perrotta
Copy link
Contributor

unhold

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

None yet

7 participants