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

Electron validation miniAOD V0 80X #12895

Merged
merged 69 commits into from Feb 2, 2016
Merged

Electron validation miniAOD V0 80X #12895

merged 69 commits into from Feb 2, 2016

Conversation

archiron
Copy link
Contributor

@archiron archiron commented Jan 8, 2016

New version of the PR12272. All was made with CMSSW_8_0_0_pre4

miniAOD validation is introduced. New modules have been added (electronMcMiniAODSignalValidator and electronMcMiniAODSignalPostValidator) with _cfg files and _cfi files.
BuildFile.xml and SealModule.cc were updated.
Minor implementations were added into ElectronMcSignalValidator.cc to take into account miniAOD differences. With this histos, it is possible to compare miniAOD vs RECO .

Some minors corrections have been made on "classical" files sur as OvalFile, electronCompare.C..

@beaudett

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2016

A new Pull Request was created by @archiron (Chiron) for CMSSW_8_0_X.

It involves the following packages:

Configuration/StandardSequences
DQMOffline/EGamma
Validation/Configuration
Validation/RecoEgamma

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @richard-cms, @cerati, @dgulhan, @rociovilar this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@davidlt
Copy link
Contributor

davidlt commented Jan 8, 2016

Why do you have so many commits with exact same message and each commit with a single file (checked only a few commits)?

@archiron
Copy link
Contributor Author

archiron commented Jan 8, 2016

by same msg do you mean with same comment "web page electron modification" for example ? If yes, this is because the modifications are only of 2 types : minor modifications for wab pages monitoring the histos (web page electron modification) and a more important modifcation with miniAOD implementation ("miniAOD electron modification)

@slava77
Copy link
Contributor

slava77 commented Jan 8, 2016

only 40 commits is still better than over 200 in the previous PR.
Still, if the message in the commit is the same, it makes more sense to squash the commits by running "git rebase -i CMSSW_yourCurrentVersion"

@davidlt
Copy link
Contributor

davidlt commented Jan 8, 2016

Instead make two commits with two different changes. Then subject line could contains "miniAOD electron modification" (but I would make it more specific) and then add a proper description in commit message (which you have in PR). Otherwise I consider this trashing GIT history (making it harder read).

Decent commit message example: rpm-software-management/rpm@a655cee

Some reading:
https://wiki.openstack.org/wiki/GitCommitMessages
http://chris.beams.io/posts/git-commit/
you can find more

@slava77
Copy link
Contributor

slava77 commented Jan 15, 2016

@deguio
this is mainly in the DQM area
please check

@deguio
Copy link
Contributor

deguio commented Jan 15, 2016

please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

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

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step2_TTbar+TTbarFS+HARVESTFS.log

135.4 step2

runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step2_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

9.0 step4

runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step4_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log

25.0 step4

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

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

@cmsbuild
Copy link
Contributor

Pull request #12895 was updated. @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@archiron
Copy link
Contributor Author

corrections have been made to take into account some typing errors.
tests on CMSSW_8_0_0_pre2 validation is OK.
tests on 5.1 and 1332.0 are OK too

@cmsbuild
Copy link
Contributor

Pull request #12895 was updated. @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@archiron
Copy link
Contributor Author

Preparing to run 135.4 ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS
135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS Step0-PASSED Step1-PASSED Step2-PASSED - time date Mon Jan 18 15:33:07 2016-date Mon Jan 18 15:13:39 2016; exit: 0 0 0
1 1 1 tests passed, 0 0 0 failed

Preparing to run 9.0 Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST
9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED - time date Mon Jan 18 15:47:18 2016-date Mon Jan 18 15:33:44 2016; exit: 0 0 0 0
1 1 1 1 tests passed, 0 0 0 0 failed

Preparing to run 25.0 TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT
25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT Step0-PASSED Step1-PASSED Step2-PASSED Step3-PASSED Step4-PASSED - time date Mon Jan 18 16:01:13 2016-date Mon Jan 18 15:47:24 2016; exit: 0 0 0 0 0
1 1 1 1 1 tests passed, 0 0 0 0 0 failed

@deguio
Copy link
Contributor

deguio commented Jan 19, 2016

please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

-1
Tested at: 8d12218
I found an error when building:

>> Compiling edm plugin /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/PhotonValidatorMiniAOD.cc 
>> Compiling edm plugin /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/SealModule.cc 
>> Compiling edm plugin /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/TkConvValidator.cc 
>> Compiling edm plugin /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoHI/plugins/HiBasicGenTest.cc 
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/ElectronMcMiniAODSignalValidator.cc: In member function 'virtual void ElectronMcSignalValidatorMiniAOD::analyze(const edm::Event&, const edm::EventSetup&)':
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/ElectronMcMiniAODSignalValidator.cc:374:59: error: expected ',' or ';' before ')' token
             double one_over_pt = 1. / bestGsfElectron.pt());
                                                           ^
In file included from /afs/cern.ch/cms/sw/ReleaseCandidates/vol1/slc6_amd64_gcc493/external/clhep/2.2.0.4-kpegke/include/CLHEP/Units/GlobalPhysicalConstants.h:2:0,
                 from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_8_0_X_2016-01-28-2300/src/Validation/RecoEgamma/plugins/ElectronMcMiniAODSignalValidator.cc:24:
/afs/cern.ch/cms/sw/ReleaseCandidates/vol1/slc6_amd64_gcc493/external/clhep/2.2.0.4-kpegke/include/CLHEP/Units/PhysicalConstants.h: At global scope:


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

@deguio
Copy link
Contributor

deguio commented Feb 1, 2016

a parenthesis needs a fix

@deguio
Copy link
Contributor

deguio commented Feb 1, 2016

@archiron could you provide the one character fix today? I will approve after.
thanks,
F.

removing parenthesis on line 374
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

Pull request #12895 was updated. @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@deguio
Copy link
Contributor

deguio commented Feb 1, 2016

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

@deguio
Copy link
Contributor

deguio commented Feb 1, 2016

+1

@cvuosalo
Copy link
Contributor

cvuosalo commented Feb 1, 2016

+1

For #12895 2fc6060

Second approval after a small code change. Jenkins tests are still OK.

davidlange6 added a commit that referenced this pull request Feb 2, 2016
@davidlange6 davidlange6 merged commit 0faefab into cms-sw:CMSSW_8_0_X Feb 2, 2016
@archiron archiron deleted the ElectronValidationMiniAOD_V0_80X branch July 19, 2019 08:27
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

9 participants