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

strip digitizer stores tofBin, hitAssociator uses CFpos #4520

Merged
2 commits merged into from Jul 10, 2014

Conversation

wmtford
Copy link
Contributor

@wmtford wmtford commented Jul 3, 2014

With this PR we fully restore the hit association functionality to what it was before CMSSW_6_2_0_pre8. That is, for the strip tracker we follow the digiSimLink directly back to the simHit. This is in contrast to the previous fix (PRs 2279, 2285, 3021, 3637), where the match was indirect through the simTrack, and is sometimes ambiguous.
Here we add a bit to the stripDigiSimLink to specify the Low- vs High-Tof collection, which in combination with the detID.subDet allows the collection-relative pointers to be used. The digitizer supplies this information on creation of the digiSimLink. The hitAssociator now uses this information. I hope I've implemented this in a way that preserves the thread-safe behavior of the hitAssociator.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2014

A new Pull Request was created by @wmtford for CMSSW_7_2_X.

strip digitizer stores tofBin, hitAssociator uses CFpos

It involves the following packages:

SimDataFormats/TrackerDigiSimLink
SimTracker/SiStripDigitizer
SimTracker/TrackerHitAssociation

@cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @rovere, @VinInn, @cnuttens, @cerati, @threus, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

@civanch
Copy link
Contributor

civanch commented Jul 4, 2014

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

@@ -6,11 +6,16 @@

class StripDigiSimLink {
public:
StripDigiSimLink(unsigned int ch, unsigned int tkId, unsigned int counter, EncodedEventId e,float a ):
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: do we allow sim data format changes in 72X wrt 71X?
@davidlange6

@wmtford
Copy link
Contributor Author

wmtford commented Jul 6, 2014

I realized that I should add protection against an out-of-range index in case the hitAssociator is taking data from prompt collections when the digitization had inserted pileup into the crossing frame. So I committed that change as noted above. I don't know if such commits made while the PR approval is pending get picked up seamlessly or not.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 6, 2014

Pull request #4520 was updated. @cmsbuild, @civanch, @Degano, @mdhildreth, @nclopezo can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes or unless it breaks tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2014

-1
Tested at: e7506a8
When I ran the RelVals I found an error in the following worklfows:
4.22 step2

runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log

4.53 step2

runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT.log

1000.0 step2

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD.log

1003.0 step2

runTheMatrix-results/1003.0_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM/step2_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM.log

50101.0 step2

runTheMatrix-results/50101.0_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID/step2_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID.log

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4520/289/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (but tests are reportedly failing).

@wmtford
Copy link
Contributor Author

wmtford commented Jul 8, 2014

If I look at the last of the failed tests, I see that the dasquery step executes the query
file dataset=/RelValSingleMuPt10/CMSSW_6_2_0_pre8-PRE_ST62_V8_FastSim-v1/GEN-SIM-DIGI-RECO site=T2_CH_CERN
Which results in the following content in step1_dasquery.log:
None
Showing 1-10 out of 1 results, for more results use --idx/--limit options

When I run the same query myself, I get
File: /store/relval/CMSSW_6_2_0_pre8/RelValSingleMuPt10/GEN-SIM-DIGI-RECO/PRE_ST62_V8_FastSim-v1/00000/70623A94-DAE0-E211-8AE6-003048F1C7D2.root
File size: 2.8GB, Site: T2_CH_CERN

File: /store/relval/CMSSW_6_2_0_pre8/RelValSingleMuPt10/GEN-SIM-DIGI-RECO/PRE_ST62_V8_FastSim-v1/00000/74C2A876-DFE0-E211-BAFA-BCAEC5329703.root
File size: 1.8GB, Site: T2_CH_CERN

File: /store/relval/CMSSW_6_2_0_pre8/RelValSingleMuPt10/GEN-SIM-DIGI-RECO/PRE_ST62_V8_FastSim-v1/00000/88951C57-DBE0-E211-A14B-003048F0117C.root
File size: 3.7GB, Site: T2_CH_CERN

For the other failed workflows the step1_dasquery.log file is empty.

The step2 failures indicate an empty list of files to run on.
Could this have been a transient problem with access to the data store?
Can these tests be rerun?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 9, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ghost pushed a commit that referenced this pull request Jul 10, 2014
strip digitizer stores tofBin, hitAssociator uses CFpos
@ghost ghost merged commit f1c6828 into cms-sw:CMSSW_7_2_X Jul 10, 2014
@davidlange6
Copy link
Contributor

@wmtford, all - it seems that this pull request has a bit adverse timing effect on stripRecHitsValid and globalrechitsanalysis modules when running on pileup events (maybe always, but we noticed on the pileup workflows, e.g., 25203, which now take minutes per event).

We will back out for now - as this would prevent us from doing any pileup relvals.

@wmtford
Copy link
Contributor Author

wmtford commented Jul 15, 2014

Hi David,

I think I see where the problem is. Now to seek a cure. The time is
being spent copying vectors of PSimHits, which is done in both the old
and new methods. But in the new method, the vectors are much larger.
It must be possible to avoid doing this. More later.

Bill

On 7/14/14, 3:18 AM, David Lange wrote:

@wmtford https://github.com/wmtford, all - it seems that this pull
request has a bit adverse timing effect on stripRecHitsValid and
globalrechitsanalysis modules when running on pileup events (maybe
always, but we noticed on the pileup workflows, e.g., 25203, which now
take minutes per event).

We will back out for now - as this would prevent us from doing any
pileup relvals.


Reply to this email directly or view it on GitHub
#4520 (comment).

@ghost ghost mentioned this pull request Jul 16, 2014
This pull request was closed.
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

6 participants