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

Sibling Doublets for the HGCal Pattern Recoginition by CA #36171

Merged
merged 6 commits into from Dec 14, 2021

Conversation

waredjeb
Copy link
Contributor

PR description:

This pull request introduces the Sibling Doublets for the HGCal Pattern Recognition.
This feature has been already discussed in the last Reconstruction and Analysis Tools meeting (link to presentation, slide 14)

  • Some validation plot here, tiny and understood changes in physics
  • Timing and memory consumption evaluated on a pileup 200 workflow (34902.999)

This PR

FastReport --------------------------- Run 1 Summary ---------------------------
FastReport   CPU time avg.      when run  Real time avg.      when run     Alloc. avg.      when run   Dealloc. avg.      when run  Modules
FastReport        47.7 ms        47.7 ms        47.8 ms        47.8 ms      +26779 kB      +26779 kB      -13708 kB      -13708 kB    ticlLayerTileProducer
FastReport         0.0 ms         0.0 ms         0.0 ms         0.0 ms          +0 kB          +0 kB          +0 kB          +0 kB    ticlSeedingGlobal
FastReport         4.5 ms         4.5 ms         4.5 ms         4.5 ms         +53 kB         +53 kB         -26 kB         -26 kB    ticlSeedingTrk
FastReport         1.1 ms         1.1 ms         1.2 ms         1.2 ms      +10846 kB      +10846 kB       -9821 kB       -9821 kB    ticlSimTracksters
FastReport       822.9 ms       822.9 ms       822.9 ms       822.9 ms     +239437 kB     +239437 kB     -237668 kB     -237668 kB    ticlTrackstersEM
FastReport        97.0 ms        97.0 ms        97.1 ms        97.1 ms      +49810 kB      +49810 kB      -48358 kB      -48358 kB    ticlTrackstersHAD
FastReport       435.9 ms       435.9 ms       435.9 ms       435.9 ms      +72455 kB      +72455 kB      -69580 kB      -69580 kB    ticlTrackstersMerge
FastReport       678.0 ms       678.0 ms       678.0 ms       678.0 ms     +163675 kB     +163675 kB     -161092 kB     -161092 kB    ticlTrackstersTrk
FastReport       520.1 ms       520.1 ms       523.4 ms       523.4 ms     +188421 kB     +188421 kB     -186847 kB     -186847 kB    ticlTrackstersTrkEM

Total Time = 2610.3 ms
### Release
FastReport --------------------------- Run 1 Summary ---------------------------
FastReport   CPU time avg.      when run  Real time avg.      when run     Alloc. avg.      when run   Dealloc. avg.      when run  Modules
FastReport        46.5 ms        46.5 ms        46.5 ms        46.5 ms      +26779 kB      +26779 kB      -13708 kB      -13708 kB    ticlLayerTileProducer
FastReport         0.0 ms         0.0 ms         0.0 ms         0.0 ms          +0 kB          +0 kB          +0 kB          +0 kB    ticlSeedingGlobal
FastReport         4.5 ms         4.5 ms         4.5 ms         4.5 ms         +53 kB         +53 kB         -26 kB         -26 kB    ticlSeedingTrk
FastReport         1.1 ms         1.1 ms         1.1 ms         1.1 ms      +10846 kB      +10846 kB       -9821 kB       -9821 kB    ticlSimTracksters
FastReport       944.4 ms       944.4 ms       944.5 ms       944.5 ms     +349877 kB     +349877 kB     -348249 kB     -348249 kB    ticlTrackstersEM
FastReport       100.5 ms       100.5 ms       100.6 ms       100.6 ms      +60440 kB      +60440 kB      -59009 kB      -59009 kB    ticlTrackstersHAD
FastReport       446.8 ms       446.8 ms       446.9 ms       446.9 ms      +72579 kB      +72579 kB      -70559 kB      -70559 kB    ticlTrackstersMerge
FastReport       719.3 ms       719.3 ms       719.3 ms       719.3 ms     +176474 kB     +176474 kB     -174532 kB     -174532 kB    ticlTrackstersTrk
FastReport       534.6 ms       534.6 ms       535.7 ms       535.7 ms     +205419 kB     +205419 kB     -203905 kB     -203905 kB    ticlTrackstersTrkEM

Total Time =  2799.3ms

@rovere @felicepantaleo @cseez @pfs

const std::vector<reco::CaloCluster> &layerClusters,
float maxRSquared) {
float etaDiff = layerClusters[outerIdx].eta() - layerClusters[innerIdx].eta();
float phiDiff = layerClusters[outerIdx].phi() - layerClusters[innerIdx].phi();
Copy link
Contributor

Choose a reason for hiding this comment

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

will this bring wraparound issues? maybe better to use a deltaR function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! replaced with deltaR2 method

@slava77
Copy link
Contributor

slava77 commented Nov 18, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36171/26729

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @waredjeb (Wahid Redjeb) for master.

It involves the following packages:

  • RecoHGCal/TICL (upgrade, reconstruction)

@jpata, @AdrianoDee, @srimanob, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @rovere, @apsallid, @sobhatta, @lecriste, @hatakeyamak, @ebrondol 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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36171/26732

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

Pull request #36171 was updated. @jpata, @AdrianoDee, @srimanob, @cmsbuild, @slava77 can you please check and sign again.

tmp.edges().reserve(ntuplet.size());
for (auto const &t : ntuplet) {
std::array<unsigned int, 2> edge = {
{(unsigned int)doublets[t].innerClusterId(), (unsigned int)doublets[t].outerClusterId()}};
Copy link
Contributor

Choose a reason for hiding this comment

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

from 4.15 http://cms-sw.github.io/cms_coding_rules.html#4--technical-coding-rules-1

Suggested change
{(unsigned int)doublets[t].innerClusterId(), (unsigned int)doublets[t].outerClusterId()}};
{static_cast<unsigned int>(doublets[t].innerClusterId()), static_cast<unsigned int>(doublets[t].outerClusterId())}};

@@ -29,6 +29,7 @@
shower_start_max_layer = 5, #inclusive
max_out_in_hops = 1,
skip_layers = 2,
siblings_maxRSquared = [6e-4, 6e-4, 6e-4],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
siblings_maxRSquared = [6e-4, 6e-4, 6e-4],

not needed here (the same as the default already).

Same comment for the other files below

@apsallid
Copy link
Contributor

Hi @waredjeb ,

If @rovere and @felicepantaleo agree and since you are in there please correct outerR() here. I understand that it isn't used anywhere but it should return outerR_ not outerZ_. Sorry for the out of topic!

@swagata87
Copy link
Contributor

Hi,

  • Do I understand correctly that its only the PF electrons / PF photons where we see significant changes, but the GED electrons / GED photons do not show any significant change?
  • Was there any study made on PU=0 sample? If yes, do we see differences for PF ele/pho in PU=0 sample as well?
  • Number of PF photon went down, in ttbar sample. As ttbar sample does not have real photons, this could mean that jet-faking-photon rate has decreased. If that's the case, then it's fine. But is there any study supporting it?
  • Apart from that one slide in the RECO meeting talk, is there any other presentation on this topic that I can look at, to know more about the motivation of this PR and the changes expected for superclusters, electrons and photons?

@slava77
Copy link
Contributor

slava77 commented Nov 29, 2021

@waredjeb @rovere
please check #36171 (comment) and clarify on the physics expectations in this PR.
Thank you.

@rovere
Copy link
Contributor

rovere commented Nov 29, 2021

@slava77 thanks for the reminder!
@waredjeb is working on this.

@felicepantaleo
Copy link
Contributor

enable profiling

@felicepantaleo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-af02b3/20966/summary.html
COMMIT: 0e21ba7
CMSSW: CMSSW_12_2_X_2021-12-03-1100/slc7_amd64_gcc900
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36171/20966/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: 5418 differences found in the comparisons
  • DQMHistoTests: Total files compared: 41
  • DQMHistoTests: Total histograms compared: 3041955
  • DQMHistoTests: Total failures: 24373
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3017559
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 40 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 175 log files, 37 edm output root files, 41 DQM output files
  • TriggerResults: no differences found

@swagata87
Copy link
Contributor

@waredjeb has made some further studies on photons (he can link his plots here, for record).
Fakerate of GED photons got significantly reduced after this PR. However, there is a small cost of efficiency. The observation of less number of reconstructed photons after this PR is mostly driven by reduced fakerate.

Given the improved fakerate of photons, and also gain in timing[1], the small efficiency loss seems tolerable to me.

[1]
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-af02b3/20966/profiling/23434.21/RES_CPU_compare_23434.21.txt

[3    -> 4   ] [4.2    -> 3.6   ]  [58.32     -> 49.54    ]         [ 49.54    /1372.46   - 58.32    /1374.99   *100% ] = [-0.63 % ]         TrackstersProducer::produce  

@smuzaffar smuzaffar modified the milestones: CMSSW_12_2_X, CMSSW_12_3_X Dec 6, 2021
@slava77
Copy link
Contributor

slava77 commented Dec 7, 2021

+reconstruction

for #36171 0e21ba7

  • code changes are in line with the PR description and the follow up review; including the more detailed physics check as summarised in Sibling Doublets for the HGCal Pattern Recoginition by CA #36171 (comment)
  • jenkins tests pass and comparisons with the baseline show differences in phase-2 workflows starting in HGCAL/trackster objects and propagating downstream
  • the profiling plots show around 0.8% reduction in CPU time, driven mainly by the speedup in TrackstersProducer::produce

@srimanob
Copy link
Contributor

srimanob commented Dec 9, 2021

@waredjeb @swagata87
Would you mind putting your study plot here for reference? Thanks.

@waredjeb
Copy link
Contributor Author

waredjeb commented Dec 9, 2021

Hi @srimanob, sure. Here you can find the new plots https://wredjeb.web.cern.ch/HGCAL/PRTest/plots/, workflow 35252.0 PU200.
Some additional information for these plots:

The photons fake rate increases a bit with this PR. We had an offline discussion with Soham, who said that given the CPU time reduction, the small changes are fine.

@slava77
Copy link
Contributor

slava77 commented Dec 13, 2021

@cms-sw/upgrade-l2
please check if this PR needs further info or if it can be signed soon.

@srimanob
Copy link
Contributor

+Upgrade

Slightly change of photon fake rate is expected from this PR.

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

@qliphy
Copy link
Contributor

qliphy commented Dec 14, 2021

+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