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

use std::vector<DetId> as data format of SiStrip DetIds with FED errors #42662

Merged
merged 3 commits into from
Aug 26, 2023

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Aug 25, 2023

PR description:

This PR changes the data format of the collection used for the DetIds of SiStrip modules with FED errors (this collection is used to mask said modules during the event reconstruction).

#42475 showed that this information needs to be added to the RawPrime event content (see that PR for details).

The current data format for this is DetIdCollection, and concerns were raised (see #42495 (comment) and replies) on the forward-compatibility of this data format in view of possibly adding it to the RAW data tier (which is expected to be backward- and forward-compatible across release cycles).

Following the discussion in #42495 (comment), this PR tries to implement a solution replacing DetIdCollection (== edm::EDCollection<DetId>) with DetIdVector (== std::vector<DetId>) in the SiStrip unpacker and related clients/utilities, such that the data format added to RAW(Prime) would be a std::vector<DetId> rather than a edm::EDCollection<DetId>.

Merely technical. No changes expected.

Thanks to @mmusich for help with preparing this PR.

PR validation:

Not much. Tested addOnTests on MC, and a few runTheMatrix wfs.

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:

CMSSW_13_2_X

Related to the 2023 HIon data-taking.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42662/36725

  • This PR adds an extra 144KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

  • CalibTracker/SiStripHitEfficiency (alca)
  • Calibration/TkAlCaRecoProducers (alca)
  • Configuration/EventContent (operations)
  • Configuration/Skimming (pdmv)
  • DPGAnalysis/SiStripTools (analysis)
  • DPGAnalysis/Skims (pdmv)
  • DQM/SiPixelHeterogeneous (dqm)
  • DataFormats/DetId (simulation)
  • EventFilter/SiStripRawToDigi (reconstruction)
  • RecoLocalTracker/Configuration (reconstruction)
  • RecoTracker/MeasurementDet (reconstruction)
  • SimG4Core/Configuration (simulation)

@rappoccio, @bbilin, @pmandrik, @tjavaid, @perrotta, @civanch, @cmsbuild, @miquork, @antoniovilela, @mandrenguyen, @davidlange6, @consuegs, @emanueleusai, @mdhildreth, @AdrianoDee, @syuvivida, @micsucmed, @fabiocos, @francescobrivio, @nothingface0, @clacaputo, @saumyaphor4252, @sunilUIET, @rvenditti, @tvami can you please review it and eventually sign? Thanks.
@echabert, @VourMa, @felicepantaleo, @yduhm, @robervalwalsh, @Martin-Grunewald, @bsunanda, @fioriNTU, @threus, @mmusich, @slomeo, @venturia, @youyingli, @makortel, @JanFSchulte, @dgulhan, @jlidrych, @GiacomoSguazzoni, @rovere, @VinInn, @tocheng, @jandrea, @alesaggio, @idebruyn, @ebrondol, @mtosi, @fabiocos, @AnnikaStein, @gbenelli, @gpetruc this is something you requested to watch as well.
@perrotta, @dpiparo, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor

fwyzard commented Aug 25, 2023

what about just changing the definition of DetIdCollection from

typedef edm::EDCollection<DetId> DetIdCollection;

to

using DetIdCollection = std::vector<DetId>;

?

Would that be more or less intrusive?

@mmusich
Copy link
Contributor

mmusich commented Aug 25, 2023

Would that be more or less intrusive?

certainly cheaper in term of changeset, but what if we want to go back to re-analyze the old persisted data (e.g. in ALCARECO samples) taken in < 13.2.X with a new release? With this solution (if we need it) we could at least plug some sort of converter upfront.

@fwyzard
Copy link
Contributor

fwyzard commented Aug 25, 2023

what if we want to go back to re-analyze the old persisted data (e.g. in ALCARECO samples) taken in < 13.2.X with a new release? With this solution (if we need it) we could at least plug some sort of converter upfront.

Good point, thanks.

@fwyzard
Copy link
Contributor

fwyzard commented Aug 25, 2023

please test

@mandrenguyen
Copy link
Contributor

urgent

Needed for PbPb data taking

@fwyzard
Copy link
Contributor

fwyzard commented Aug 25, 2023

These changes break compatibility with old AlCaRaw data.

This is expected, but it affects some of the unit tests: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17e62e/34486/unitTests/src/CalibTracker/SiStripHitEfficiency/test/testSiStripHitEfficiency/testing.log .

How do people want to proceed ?

@mmusich
Copy link
Contributor

mmusich commented Aug 25, 2023

How do people want to proceed ?

Ideally we should detect at rutime which format we have to deal with and act consequently. If we are short in time we can silence the tests for now while we figure how to support the old data.

missirol added a commit to missirol/cmssw that referenced this pull request Aug 25, 2023
missirol added a commit to missirol/cmssw that referenced this pull request Aug 25, 2023
@mandrenguyen
Copy link
Contributor

+reconstruction

@emanueleusai
Copy link
Member

+1

@perrotta
Copy link
Contributor

+alca

@perrotta
Copy link
Contributor

@cms-sw/simulation-l2 @cms-sw/pdmv-l2 please have a look at this PR

@perrotta
Copy link
Contributor

+operations

@civanch
Copy link
Contributor

civanch commented Aug 26, 2023

+1

@perrotta
Copy link
Contributor

+1

  • @cms-sw/pdmv-l2 let merge this in the master, so that it can be tested in the IBs. Please comment (and sign, if you think so) on the backported PR

@perrotta
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 1ad82dd into cms-sw:master Aug 26, 2023
@missirol missirol deleted the devel_stdVecForDetIdsOfSiStripDigis branch August 26, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment