Remove duplicate pixels

[ferencek]

PR description:

This PR completely removes all duplicate pixels from the clustering step. This is an alternative to #37496 where charges from duplicate pixels are summed up.

No changes are expected in MC samples because duplicate pixels are not expected to appear in simulation. Nevertheless, as discussed below in #37559 (comment) and follow-up comments, it was observed that duplicate pixels actually do appear in Phase 2 MC workflows. If the intention was to add up charges from such duplicate pixels, this is not done even in the current clusterizer code and instead only the last occurrence of a duplicate pixel is taken. Hence, this issue will have to be addressed on the Phase 2 pixel digitizer side.

In the data there could be some tiny differences in case some duplicate pixels appear.

PR validation:

Code compiles :)

[mmusich]

for my own education, what does this mean?

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37559/29301

• This PR adds an extra 24KB to repository

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

  • File RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc modified in PR(s): [DO NOT MERGE] Add duplicate pixels #37496

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

• code-format:
  https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37559/29301/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37559/29301/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[ferencek]

I was also wondering what future-commit meant and why the code check were not immediately triggered.

Concerning the code format checks, I actually ran scram build code-format with the following output

[ferencek@titan src]$ scram build code-format
>> Local Products Rules ..... started
>> Local Products Rules ..... done
gmake[1]: Entering directory '/users/ferencek/PixelOffline/DuplicatePixels/CMSSW_12_4_0_pre2'
>> Local Products Rules ..... started
>> Local Products Rules ..... done
/users/ferencek/PixelOffline/DuplicatePixels/CMSSW_12_4_0_pre2/config/SCRAM/find-extensions.sh: line 14: cd: /users/ferencek/PixelOffline/AddDuplicatePixels/CMSSW_12_4_0_pre2: No such file or directory
clang-format -i src/DataFormats/SiPixelCluster/interface/SiPixelCluster.h
clang-format -i src/RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc
clang-format -i src/RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.h
gmake[1]: Leaving directory '/users/ferencek/PixelOffline/DuplicatePixels/CMSSW_12_4_0_pre2'

so the command apparently did not do anything. The confusing line is

/users/ferencek/PixelOffline/DuplicatePixels/CMSSW_12_4_0_pre2/config/SCRAM/find-extensions.sh: line 14: cd: /users/ferencek/PixelOffline/AddDuplicatePixels/CMSSW_12_4_0_pre2: No such file or directory

which possibly explains why the code format checks somehow silently failed. I say confusing because I did run scram b ProjectRename after renaming the parent folder. There were some other issues with project renaming which I reported in #37567

[davidlange6]

the message means what it says I think- the commit has a timestamp after the timestamp of the pull request (21:23 vs 21:14). perhaps the computer of @ferencek has the wrong time?
https://api.github.com/repos/CMSTrackerDPG/cmssw/commits/d50b2f5b53d771a27abf11e34b6f9de89f330bda
https://api.github.com/repos/cms-sw/cmssw/issues/37559

[ferencek]

the message means what it says I think- the commit has a timestamp after the timestamp of the pull request (21:23 vs 21:14). perhaps the computer of @ferencek has the wrong time? https://api.github.com/repos/CMSTrackerDPG/cmssw/commits/d50b2f5b53d771a27abf11e34b6f9de89f330bda https://api.github.com/repos/cms-sw/cmssw/issues/37559

Thanks, @davidlange6, for catching this. Indeed, the server machine I've been working on had an NTP synchronization turned off for some reason and the system clock drifted forward in time.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37559/29309

• This PR adds an extra 24KB to repository

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

  • File RecoLocalTracker/SiPixelClusterizer/plugins/PixelThresholdClusterizer.cc modified in PR(s): [DO NOT MERGE] Add duplicate pixels #37496

[cmsbuild]

A new Pull Request was created by @ferencek (Dinko F.) for master.

It involves the following packages:

• DataFormats/SiPixelCluster (reconstruction)
• RecoLocalTracker/SiPixelClusterizer (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@mtosi, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @OzAmram, @ferencek, @dkotlins, @gpetruc, @mmusich, @threus, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[tsusa]

please test

[jpata]

Is this one also DO NOT MERGE, like #37496?

[ferencek]

Is this one also DO NOT MERGE, like #37496?

I hope this one is for merging if all tests are successful.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e40131/26705/summary.html
COMMIT: 299d266
CMSSW: CMSSW_12_5_X_2022-08-08-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37559/26705/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 6089 differences found in the comparisons
• DQMHistoTests: Total files compared: 51
• DQMHistoTests: Total histograms compared: 3691612
• DQMHistoTests: Total failures: 51731
• DQMHistoTests: Total nulls: 16
• DQMHistoTests: Total successes: 3639843
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: -0.004 KiB( 50 files compared)
• DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
• Checked 212 log files, 49 edm output root files, 51 DQM output files
• TriggerResults: no differences found

[connorpa]

+1

[mmusich]

@cms-sw/reconstruction-l2 is there anything still outstanding here?

[jpata]

Thanks for the updates.
There are a large number of very small changes, also in MC, in tracks, PF, MET, BTV.

No changes are expected in MC samples

Is this statement in the PR description now outdated?

[mmusich]

@jpata

No changes are expected in MC samples
Is this statement in the PR description now outdated?

it appears you totally missed the discussion at #37559 (comment) and following. Please have a look.

[fwyzard]

I did miss it :-/

...and I was assuming we would not need the duplicate removal for Phase-2, also because the modules are too large to use the same algorithm I implemented for Phase-1 :-(

What should we do for Phase-2 then ?
Change the GPU code to sum the duplicates ?
Or change the digitizer to avoid producing duplicates ?

[jpata]

it appears you totally missed the discussion at #37559 (comment) and following. Please have a look.

Thanks for the reminder. @ferencek can the PR description be updated to reflect that discussion?

[mmusich]

What should we do for Phase-2 then ?
Change the GPU code to sum the duplicates ?
Or change the digitizer to avoid producing duplicates ?

IMHO for the time being we should not do anything.
The ROC of the Phase2 pixel will be totally different and we (at least I) don't know if it can (or will) produce duplicate pixels.
In the longer term (as I suggested above) - certainly longer than the target of this PR (~ urgent for Run3 consistency ), I think the digitizer logic should be changed to avoid duplicate simDigis and just sum them up upstream of any recontruction.

[mmusich]

I think the digitizer logic should be changed to avoid duplicate simDigis and just sum them up upstream of any recontruction.

In the meanwhile this can become an open cmssw issue with simulation signature

[ferencek]

it appears you totally missed the discussion at #37559 (comment) and following. Please have a look.

Thanks for the reminder. @ferencek can the PR description be updated to reflect that discussion?

Done. I hope it's fine.

[jpata]

+reconstruction

• urgently fixes duplicate pixels for Run3 offline, for consistency with the HLT GPU code
• these only occur in data (hence differences in Run2 data workflows)
• differences in Phase2 MC can be expected due to simulation: Phase2 duplicate pixels handling #39007

[cmsbuild]

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

[qliphy]

+1