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

SiStrip Calibration common framework revamp #13101

Merged
merged 15 commits into from Feb 3, 2016

Conversation

mverzett
Copy link
Contributor

This PR revamps the SiStrip Calibration common framework. It also slightly changes the ALCARECO workflow to be more DRY and have overlapping code for PCL and standalone calibration workflows.

I also added several unit tests to check in the IBs whether some upstream code changes may break our code and act promptly. Would it be possible to add this script to the jenkins stack?

Interested people: @dimattia @jlagram

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mverzett (Mauro Verzetti) for CMSSW_8_0_X.

It involves the following packages:

CalibTracker/SiStripChannelGain
CalibTracker/SiStripCommon
Calibration/TkAlCaRecoProducers

@diguida, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@jlagram, @threus, @gbenelli, @tocheng this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@mmusich
Copy link
Contributor

mmusich commented Jan 28, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10797/console

@mmusich
Copy link
Contributor

mmusich commented Jan 28, 2016

@mverzett about the request for addition of the unit tests I suggest you open an issue in the cmssw project

@boudoul
Copy link
Contributor

boudoul commented Jan 28, 2016

Hi @mverzett , could you please make a better description in the title, 'New strip sw' is not very explicit .
Also we are encouraging you to add README.md files per package in order to describe the packages functionality (in particular what the codes you rewrote is actually doing) , could you please add it ?
Thanks a lot

@mverzett mverzett mentioned this pull request Jan 28, 2016
@mverzett
Copy link
Contributor Author

Hi @boudoul, sure! I will start working on it. We planned to have a Twiki, but a README is just as good. If there are no objections I would go for a merge of this PR and I will provide extensive documentation in a future one (I don't want to miss the release deadline).

@mmusich new issue created

@boudoul
Copy link
Contributor

boudoul commented Jan 28, 2016

hi @mverzett , thanks a lot, that's perfect

@cmsbuild
Copy link
Contributor

@@ -22,7 +25,7 @@ produce(edm::Event& iEvent, const edm::EventSetup& iSetup) {


edm::Handle< L1GlobalTriggerReadoutRecord > gtRecord;
iEvent.getByLabel( edm::InputTag("gtDigis"), gtRecord);
iEvent.getByToken(trig_token_, gtRecord);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidlange6 change made. Thanks for spotting it!

@mverzett
Copy link
Contributor Author

mverzett commented Feb 2, 2016

The last change was to comply with @davidlange6 comment to use the token system in one module in which was missing.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

-1
Tested at: 634da0b
When I ran the RelVals I found an error in the following worklfows:
134.911 step1

DAS Error

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

@mmusich
Copy link
Contributor

mmusich commented Feb 2, 2016

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

@mmusich
Copy link
Contributor

mmusich commented Feb 2, 2016

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Feb 3, 2016
SiStrip Calibration common framework revamp
@cmsbuild cmsbuild merged commit 195b670 into cms-sw:CMSSW_8_0_X Feb 3, 2016
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