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

Fix the index in the dEdxHitAssociation #8549

Merged
merged 1 commit into from Apr 1, 2015

Conversation

quertenmont
Copy link
Contributor

Super important bug fix in the dEdxHit association.
The index used was incorrect --> making the map totally useless.
This patch fix the issue and must be used asap (also for data release).

I am therefore making a PR for both 74X and 75X

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @quertenmont (Loic Quertenmont) for CMSSW_7_5_X.

Fix the index in the dEdxHitAssociation

It involves the following packages:

RecoTracker/DeDx

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @mschrode, @gpetruc, @cerati, @dgulhan, @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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented Mar 26, 2015

Hi Loic,

is this needed for what may remain of CRAFT and other current data taking tests?
(some rereconstruction here may happen as well)
If so, you may want to submit another PR to 73X

@quertenmont
Copy link
Contributor Author

Hi Slava,

No, actually I don't think that this piece of code was already in 73X.
So no issue with that. I just want to make sure that this will be used
for beam reconstruction and Run2 MC prod.
My understanding is that 74X will be THE release, so I am happy

Loic

Le 26/03/2015 17:29, Slava Krutelyov a écrit :

Hi Loic,

is this needed for what may remain of CRAFT and other current data
taking tests?
(some rereconstruction here may happen as well)
If so, you may want to submit another PR to 73X


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

@slava77
Copy link
Contributor

slava77 commented Mar 27, 2015

+1

for #8549 e7dfb29
this is the same change as in #8548 e7dfb29, signed off for 74X.

We should get something in DQM to monitor the stored quantities from the DeDxHitInfoProducer in the standard workflows.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

@slava77
Copy link
Contributor

slava77 commented Mar 27, 2015

@cmsbuild please test
for completeness

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@davidlange6
Copy link
Contributor

On Mar 27, 2015, at 1:04 PM, Slava Krutelyov notifications@github.com wrote:

+1

for #8549 e7dfb29
this is the same change as in #8548 e7dfb29, signed off for 74X.

We should get something in DQM to monitor the stored quantities from the DeDxHitInfoProducer in the standard workflows.

since we seem to have this same discussion every six months, perhaps @deguio can work with @quertenmont to add something to this PR that at least shows basic functionality of the de/dx data formats?


Reply to this email directly or view it on GitHub.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes or unless it breaks tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

@cmsbuild
Copy link
Contributor

-1
Tested at: e7dfb29
When I ran the RelVals I found an error in the following worklfows:
25.0 step4

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT/step4_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT.log

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

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (but tests are reportedly failing). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

@slava77
Copy link
Contributor

slava77 commented Mar 27, 2015

the error is unrelated to this PR.
@deguio Federico, is this issue being resolved? (I've seen it twice already, the last one was in tests of #8515 https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8515/239/runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step4_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log

%MSG-e FatalSystemSignal:   DQMGenericClient:postProcessorMuonMultiTrackHLT@endJob  27-Mar-2015 13:45:43 CET PostEndRun
A fatal system signal has occurred: segmentation violation
%MSG

@quertenmont
Copy link
Contributor Author

This is for sure unrelated to this PR.
(I ran locally on ZMM13TeV relval sucesfully yesterday)

@quertenmont
Copy link
Contributor Author

when is the next prerelease expected?
I would really like to have this patch in.

davidlange6 added a commit that referenced this pull request Apr 1, 2015
Fix the index in the dEdxHitAssociation
@davidlange6 davidlange6 merged commit 5005b46 into cms-sw:CMSSW_7_5_X Apr 1, 2015
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

4 participants