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

Multithread pixel update - raw data error fix #3952

Merged
merged 5 commits into from Jun 25, 2014

Conversation

leggat
Copy link
Contributor

@leggat leggat commented May 21, 2014

Found the bug in the RawDataError code - two different objects were trying to access the same ME. Solved by booking the MEs in the Source code within a map passed to the Module at the fill step.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @leggat for CMSSW_7_1_X.

Multithread pixel update - raw data error fix

It involves the following packages:

DQM/SiPixelMonitorCluster
DQM/SiPixelMonitorDigi
DQM/SiPixelMonitorRawData
DQM/SiPixelMonitorRecHit
DQM/SiPixelMonitorTrack

@ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks.
@threus 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented Jun 3, 2014

ciao @leggat
sorry for being so late in reviewing your fix. I don't see exactly where different objects were trying to access the same ME, but the new implementation looks ok to me.

I just want to ask you to remove the method:
SiPixelRawDataErrorModule::bookFED

since it is no longer needed. could you also remove the commented code and add a commit to the same branch? this PR should update automatically.

thanks,
F.

@threus threus mentioned this pull request Jun 3, 2014
@leggat
Copy link
Contributor Author

leggat commented Jun 5, 2014

Hi @deguio
I've updated those things. Does everything look okay now?
Cheers,
Duncan

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2014

Pull request #3952 was updated. @ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please check and sign again.

@deguio
Copy link
Contributor

deguio commented Jun 5, 2014

hello @leggat
I was asking you to make some cleanup removing the useless code, not commenting it out. ;)
there are also other portions of the code which can be just dropped (they are commented).

everything will be easily accessible in the history.
cheers and thanks,
F.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2014

@deguio
Copy link
Contributor

deguio commented Jun 9, 2014

hello @leggat
I have noticed that the LS based histos are not properly compared. see:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_1_X_2014-06-06-0200+3952/2770/25.0_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT/comparisonResults/Pixel.html

could you please have a look and confirm that everything is ok?
thanks,
F.

@deguio
Copy link
Contributor

deguio commented Jun 10, 2014

+1
the LS-based histos are skipped by default when running relMon

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@davidlange6
Copy link
Contributor

@ktf - as already discussed as a possibility in the ORP - could you move (and accept) this PR into 72x?

@deguio
Copy link
Contributor

deguio commented Jun 10, 2014

ok with me.
thanks,
F.

@deguio
Copy link
Contributor

deguio commented Jun 12, 2014

for the record:
we are keeping this PR open waiting to see if everything is working fine in 72. the back port to 71 (this PR) will be needed for the july GR.

@deguio
Copy link
Contributor

deguio commented Jun 17, 2014

+1
ready to go in.

@leggat
Copy link
Contributor Author

leggat commented Jun 19, 2014

Hi @deguio ,
I've just been checking the SiPixelCommon code, and SiPixelFolderOrganizer needs updating to avoid the use of the DQMStore - the fix is easy, but it requires additional changes to the files in this pull request. Would you rather I updated those now and added them into this pull request, or wait for this to be merged and create a new one?

@ktf ktf modified the milestones: Next CMSSW_7_1_X, CMSSW_7_1_0 Jun 23, 2014
@deguio
Copy link
Contributor

deguio commented Jun 24, 2014

please make another PR unless you have the fix ready. this one has to go in tomorrow.
ciao and thanks,
F.

davidlange6 added a commit that referenced this pull request Jun 25, 2014
Multithread pixel update - raw data error fix
@davidlange6 davidlange6 merged commit c4d8d9b into cms-sw:CMSSW_7_1_X Jun 25, 2014
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