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

Phase 2 tracker: template CPE #28448

Merged
merged 10 commits into from Dec 6, 2019
Merged

Conversation

cmantill
Copy link
Contributor

PR description:

This PR integrates the template method for the pixel hit reconstruction in the Cluster Parameter Estimators (CPEs) for Phase 2. The templates have been produced for 5 different geometries in a "temporary" configuration that has been described in detail here [1]. The PR introduces the following changes:

  1. Provide phase 2 conditions for geometries: T5, T6, T14, T15, T16 with latest DB conditions.
  2. Remove phase 2 customizations forcing CPE generic method.
  3. Increase array sizes to allow for larger angle space of the phase 2 templates.

@mmusich @tsusa @OzAmram @tvami

PR validation:

Tested with phase 2 workflows: runTheMatrix.py --what upgrade -l 20007.0,20407.0,20807.0,21607.0,22007.0,22807.0,22834.0  -t 4 -j 8.

The template method for pixel hit residuals has been also tested running the reconstruction step for 100K events and results will be shown here [2]. Since the templates have been produced under "temporary" conditions, we do not expect a big change in the resolution results obtained with this method.

[1] https://indico.cern.ch/event/838960/contributions/3630543/attachments/1941870/3220114/CMS_phase2tem_Nov5.pdf
[2] https://indico.cern.ch/event/838961/

if this PR is a backport please specify the original PR:

NA

@cmsbuild cmsbuild changed the base branch from CMSSW_11_0_X to master November 21, 2019 22:00
@cmsbuild
Copy link
Contributor

@cmantill, CMSSW_11_0_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28448/12875

  • This PR adds an extra 100KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cmantill (Cristina Ana Mantilla Suarez) for master.

It involves the following packages:

CondFormats/SiPixelTransient
Configuration/AlCa
DQM/TrackingMonitor
RecoEgamma/EgammaElectronProducers
RecoLocalTracker/SiPixelRecHits
RecoMuon/Configuration
RecoMuon/GlobalTrackingTools
RecoMuon/MuonIdentification
RecoMuon/TrackingTools
RecoParticleFlow/PFTracking
RecoTracker/FinalTrackSelectors
RecoTracker/SpecialSeedGenerators
RecoTracker/TrackProducer

@perrotta, @andrius-k, @kmaeshima, @schneiml, @tlampen, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @jfernan2, @fioriNTU, @slava77, @ggovi, @pohsun can you please review it and eventually sign? Thanks.
@felicepantaleo, @jainshilpi, @abbiendi, @echapon, @fioriNTU, @lgray, @threus, @mmusich, @seemasharmafnal, @mmarionncern, @hdelanno, @battibass, @makortel, @jhgoh, @dgulhan, @dkotlins, @cericeci, @trocino, @rociovilar, @Sam-Harper, @cbernet, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @tocheng, @jandrea, @mschrode, @idebruyn, @ebrondol, @mtosi, @amagitte, @Martin-Grunewald, @HuguesBrun, @varuns23, @Fedespring, @calderona, @hatakeyamak, @sobhatta, @folguera, @afiqaize, @gpetruc, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Nov 21, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 21, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3587/console Started: 2019/11/21 23:12

@slava77
Copy link
Contributor

slava77 commented Nov 21, 2019

@cmantill
please clarify the scope of the "RFC".
Is this for the tracker community?
Is the software already adequate for physics or is this just a prototype which is known to need more work.
Thank you.

@fabiocos
Copy link
Contributor

@mmusich thanks for the clarification

@perrotta
Copy link
Contributor

@cmantill , I made cpu timing measurements with and without this PR on top of the baseline (it was CMSSW_11_0_X_2019-11-27-2300) and I did not notice any significant increase in time at step3. I am happy of it, of course. But could you please tell me how did you make the timing measurements in #28448 (comment), which apparently reports about a sizeable processing time increase with this PR

@cmantill
Copy link
Contributor Author

@perrotta I ran step3 with --customise Validation/Performance/TimeMemoryInfo.py, in a cluster machine at LPC with nThreads=4, and then just used the log to compare the timing with ~/tools/getTimeMemSummary.sh log, but if your tests don't show the same then is probably fine. I'd be happy to re-test with your commands if needed of course.

@perrotta
Copy link
Contributor

@perrotta I ran step3 with --customise Validation/Performance/TimeMemoryInfo.py, in a cluster machine at LPC with nThreads=4, and then just used the log to compare the timing with ~/tools/getTimeMemSummary.sh log, but if your tests don't show the same then is probably fine. I'd be happy to re-test with your commands if needed of course.

The overall job time could be affected by some normalization issue caused by other jobs running in parallel on the same machine. One should make sure that there are not concurrent jobs while running your step3.
I normally compare the cpu taken by the single modules using ~/tools/timeDiffFromReport.sh, as described in https://twiki.cern.ch/twiki/bin/viewauth/CMS/RecoIntegration#Compare_timing (and you can even apply some normalization factor there, if it is needed to pick up which module modifies its time differently than all the other ones). By doing so I didn't notice relevant time differences in the affected modules: but I would be happy if you could cross-check it with your sample

@cmantill
Copy link
Contributor Author

cmantill commented Nov 29, 2019

@perrotta Running again, and making sure other are no other concurrent jobs I actually see a slight decrease in time (might be random?)
no PR vs with PR
Job total: 5.9537 s/ev ==> 5.38926 s/ev

Here is the diff log. with ~/tools/timeDiffFromReport.sh.

And running ~/tools/getTimeMemSummary.sh I get:
with PR

Summary for 10 events
Max VSIZ 6746.36 on evt 6 ; max RSS 3729.45 on evt 9
Time av 13.4345 s/evt   max 26.6682 s on evt 4
CPU av 0 s/evt   max 0 s on evt -1
M1 Time av 12.5571 s/evt   max 26.6682 s on evt 4
M1 CPU av 0 s/evt   max 0 s on evt -1
M8 Time av 6.11572 s/evt   max 6.24101 s on evt 10

no PR

Summary for 10 events
Max VSIZ 6687.9 on evt 3 ; max RSS 3703.52 on evt 7
Time av 15.9845 s/evt   max 35.3513 s on evt 4
CPU av 0 s/evt   max 0 s on evt -1
M1 Time av 14.2675 s/evt   max 35.3513 s on evt 4
M1 CPU av 0 s/evt   max 0 s on evt -1
M8 Time av 4.01985 s/evt   max 5.0875 s on evt 9

@perrotta
Copy link
Contributor

Thank you @cmantill : this confirma my finding that timing is not affected by this PR

@perrotta
Copy link
Contributor

+1

  • CPE templates allowed for Phase2, with conditions taken from DB
  • Increased size of templates originates a manageable of the heap memory
  • CPU processing time is not affected
  • Jenkins tests pass and show minor changes in the Phase2 tracking related outputs compatible with the updates

@fabiocos
Copy link
Contributor

fabiocos commented Dec 3, 2019

@mmusich thanks for the confirmation, the specific objects modified in this PR do not seem to enter directly into the digitization (it is the SiPixelkTemplate2D object that enters for what I can see).

@christopheralanwest @tocheng @ggovi could you please check the latest update and comment or sign it?

@mmusich
Copy link
Contributor

mmusich commented Dec 3, 2019

@fabiocos

not seem to enter directly into the digitization (it is the SiPixelTemplate2D object that enters for what I can see).

right. The 2D template objects are needed for the charge re-weighting which we'll use to inject the radiation damage simulation, but this will be provided later on as the phase-2 digitizer needs to be adjusted first to consume it. This will be one milestone development for the next cycle.

@fabiocos
Copy link
Contributor

fabiocos commented Dec 5, 2019

@ggovi the changes since your signature were merely technical could you pleas echeck and sign it again? In any case I will consider your previous signature as valid
@christopheralanwest @tuos @tlampen could you please have a final check and sign it or comment?

@ggovi
Copy link
Contributor

ggovi commented Dec 5, 2019

+1

@tocheng
Copy link
Contributor

tocheng commented Dec 6, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 6, 2019

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

@fabiocos
Copy link
Contributor

fabiocos commented Dec 6, 2019

+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