-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Switching default jet collection from AK5 to AK4 #2171
Conversation
A new Pull Request was created by @rappoccio for CMSSW_7_1_X. Switching default jet collection from AK5 to AK4 It involves the following packages: CommonTools/ParticleFlow The following packages do not have a category, yet: SLHCUpgradeSimulations/Configuration @deguio, @lveldere, @cmsdoxy, @cerminar, @Martin-Grunewald, @anton-a, @perrotta, @ojeda, @vlimant, @cmsbuild, @fwyzard, @ktf, @thspeer, @rovere, @giamman, @slava77, @vadler, @Degano, @fabiocos, @nclopezo, @danduggan, @franzoni can you please review it and eventually sign? Thanks. |
I should also mention that we're keeping AK5 for consistency, but nothing downstream is supposed to use it. |
-1 TSG has not signed off on moving from ak5 to ak4 in the HLT. |
Hi Martin, all just to let you know that am confdb first trial version with the change ak5 --> ak4 is at /users/degrutto/ak4/V6 (working in 700pre11) Is has not been signed off by TSGm, it is true, I'm afraid TSG policy to signed this off requires the validation of many POG and PAG related quantities.. thanks
|
-1 ---> test runtestPhysicsToolsPatAlgos had ERRORS you can see the results of the tests here: |
Hi Michele, Absolutely, for example in the TSG meetings, today and/or Thursday. Best regards Martin |
-1 ---> test runtestPhysicsToolsPatAlgos had ERRORS you can see the results of the tests here: |
With respect to runtestPhysicsToolsPatAlgos, this is expected because this collection is not present in the RelVal that this is pointing to, but will be in the future. @vadler can confirm. |
-1 |
Apologies, it looks like my script missed a group of files. I'm testing a fix now. |
BTW: Also some short-matrix work flows break: 8.0, 4.22 |
Note : The PatAlgos test will still break due to the missing file but I checked with the "matrix" file 25.0/step3.root and it works. |
I see merge conflicts in CMSSW_7_1_X_2014-01-28-1400:
|
-1 Changes in the HLT (HLTrigger package and dependent subsystems) must go via ConfDB. Please remove HLT from this PR. |
-1 |
There is an issue in the script which tests pull requests. I've fixed it On 30 Jan 2014, at 11:53, Volker Adler wrote:
|
# print 'PF2PAT: selecting jet algorithm ', algo | ||
|
||
|
||
if algo == 'IC5': | ||
#allPfJets = RecoJets.JetProducers.ic5PFJets_cfi.iterativeCone5PFJets.clone() | ||
jetAlgo = RecoJets.JetProducers.ic5PFJets_cfi.iterativeCone5PFJets.clone() | ||
elif algo == 'AK5': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sal: shouldn't this require an additional
elif algo == 'AK4'
?
Using an ak4 jet producer when algo is 'AK5' is quite prone to mistakes and misunderstandings...
Hi, Folks, @perrotta : Yes, I will commit a fix for that now. @vadler and @ktf : what is the situation with the integration build scripts? Maybe for now can you just tell me what pull request number those changes correspond to and I can fix them? @Martin-Grunewald : I'm following the JIRA thread here : https://its.cern.ch/jira/browse/CMSHLT-47 What should I do here? Just remove the HLT configurations or wait until the CondDB is sorted out? |
-1 |
Hi, Volker, All, Sorry, the last commit was not supposed to fix the merge conflict. I managed to do that now. Please have a look. Cheers, |
Pull request #2171 was updated. @diguida, @deguio, @lveldere, @cmsdoxy, @Martin-Grunewald, @anton-a, @thspeer, @rcastello, @bendavid, @perrotta, @civanch, @ojeda, @vlimant, @cmsbuild, @fwyzard, @ktf, @davidlange6, @vciulli, @Dr15Jones, @mdhildreth, @rovere, @giamman, @slava77, @ggovi, @vadler, @Degano, @mulhearn, @apfeiffer1, @nclopezo, @danduggan, @monttj, @franzoni can you please check and sign again. |
rappoccio notifications@github.com ha scritto:
Thank you Sal.
Yes, please, remove all HLT configurations for now. Andrea
This message was sent using IMP, the Internet Messaging Program. |
The system still says 'tests-rejected' |
Hi, Chris, Those are the standard PAT tests that are expected to fail due to the missing product in the existing RelVal files. When run on a "matrix" file 25.0/step3.root it works. Cheer,s |
+1 |
+1 On Thu, Mar 13, 2014 at 5:52 PM, Chris Jones notifications@github.comwrote:
Thanks, |
@ktf - this one is at last ready for a special build = CMSSW_7_1_0_pre4 + this pull request |
The code in question seems to heavily include developments which came after pre4, am I wrong? Wouldn't it make more sense to test this with pre5 rather than me undoing those changes? |
@ktf This is exactly what we said about pre3 (to postpone until pre4). There are no developments aside from rebasing and again. This will never converge unless we just put it in. |
My bad. Things are actually ok. Sorry for the noise. @nclopezo is On 17 Mar 2014, at 14:15, rappoccio wrote:
|
Hi All, CMSSW_7_1_0_pre4_AK4 is now available. it is CMSSW_7_1_0_pre4 + this pull request |
Phew! Thanks! |
Hi David Thanks! On Mar 18, 2014, at 10:20 AM, David Mendez notifications@github.com
|
Closing…(happily) |
I announced it here: https://hypernews.cern.ch/HyperNews/CMS/get/relAnnounce/1399.html |
Merge branch 'rappoccio_AK5_To_AK4_710' of https://github.com/rappoccio/cmssw into HEAD
Update python to 2.7.11
Here I have changed the default jet collection for downstream modules from AK5 to AK4. This affects :
I have not self-ported changes of hard-coded cone sizes from 0.5 to 0.4, except in the Jet RECO packages. There are probably pieces in BTagging and TauTagging code that use cone sizes and must change for consistency.