-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Hgc digithr g4cutrange #4551
Hgc digithr g4cutrange #4551
Conversation
…r base Adding time samples (for now BX is the unit - RecHits will use the first time sample)
…Calice specialization for HEB and removing cloning of modules Adding Calice specialization to HGCHEbackDigitizer.cc Adding digitizer version to all digitizers Switching on 25ns time window for all Including OOT pu contributions in the 25ns window
A new Pull Request was created by @pfs (Pedro Silva) for CMSSW_6_2_X_SLHC. Hgc digithr g4cutrange It involves the following packages: Geometry/HGCalSimData The following packages do not have a category, yet: Geometry/HGCalSimData @civanch, @nclopezo, @mdhildreth, @cmsbuild, @Degano, @ktf can you please review it and eventually sign? Thanks. |
Testing at the moment. @pfs is this going to affect memory use? I need to run some resource checks on the HGCal digitisation (relvals failed in digi, apparantly due to memory use). Just wondering whether I should test that before or after this pull request. Looks like you're relaxing cuts so presumably memory use would increase? |
|
||
//handle sim hits | ||
int maxSimHitsAccTime_; | ||
int bxTime_; | ||
HGCSimHitDataAccumulator simHitAccumulator_; | ||
HGCSimHitDataAccumulator *simHitAccumulator_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be made a std::unique_ptr? Or left as a standard data member. There is no corresponding delete for the new. Having said that I'm pretty sure only one HGCDigitizer is instantiated (is it?) so the leak isn't that big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experienced crashes at the end of the generation of ttbar events with it declared as data member.
But I think we can make a std::unique_ptr. Per job there are 3 HGCDigitizers, one for each HGC section (EE,HEF,HEB)
Every single test, including non HGCal, fails in step3 with:
|
My bad. I forgot to propagate the removal of untracked to the RecHitProducers. |
…ducer for tracked parameters in the cfg
Only some Phase 1 scenarios pass. HGCal scenarios fail in step 2 with
The other Phase2 scenarios (and some Phase1) fail in step 3 with
Can you try a few of the workflows to make sure everything is okay? E.g. a simple set with Phase1, HGCal and Shashlik is
|
@mark-grimes, @pfs, @bsunanda Mark, I just had a run with today's IB (07-10-0200) plus this PR |
@mark-grimes, @pfs, @bsunanda There is a crash at step2 with 14816 workflow (QCD tith V4Muon, V4MuonReco) with |
@mark-grimes, @pfs, @bsunanda -l 12216 (HGC v5, QCD) gives crash at step2 in HGCalDigitizer with complaint on memory allocation: |
@mark-grimes @vandreev11 @bsunanda I think i understand why it's crashing, and I can now run the QCD generation without problems. I'm doing one final cross check with igprof. |
…r.cc Moved data members to unique_ptr which are instantiated only if the digitizer is in charge of a sub-detector (previously they where all being instantiated even if only one would be used)
I have updated the PR. I added some changes, moving data members to std::unique_ptr but that was not the fix. It was related to this line here only PFCal-dev@ba6bb59#diff-2f822c20cc09dbcda00cee6fb3a20892R174 I ran igprof on the QCD sample and i put the memory profile reports here, in case you want to take a look: /afs/cern.ch/user/p/psilva/public/PR4551 |
@mark-grimes @vandreev11 @bsunanda I clicked on s.th. i shouldn't have clicked... sorry for the noise. The PR is now re-opened. |
@mark-grimes @lgray @vandreev11 @bsunanda this now includes the optimization of the sim hit data accumulator discussed in some private thread |
@pfs @mark-grimes Now that we removed whatever the issue was for the memory footprint, you can probably switch back to using doubles if you want... Doubt it is needed though. |
@lgray doubles are not needed, indeed. I just compared the ADC spectra for single pions and they are similar. |
@pfs bon |
merge Tested with muons and pions, no problems. Haven't checked the memory yet but will do once it's in an IB. |
Updates to HGCal digitization
Changing range cut in Geant4 in Si for e/gamma to 30microns (~1/3 Si width)
@vandreev11 Can you check as well?
@bsunanda Forgot to include you to watch this PR as well.