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
Configurable Pixel Cluster Shape in LowPtSeedComparitor #17393
Configurable Pixel Cluster Shape in LowPtSeedComparitor #17393
Conversation
A new Pull Request was created by @ebrondol for CMSSW_9_0_X. It involves the following packages: RecoPixelVertexing/PixelLowPtUtilities @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
@cmsbuild please test |
The tests are being triggered in jenkins. |
Thanks @ebrondol. But I'm afraid this is not enough because LowPtClusterShapeSeedComparitor appears to be used in HLT. But we'll see if the tests succeed or fail. |
-1 Tested at: 06ac88e You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals AddOn
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step2_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log1306.0 step2 runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step2_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log1330.0 step2 runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15/step2_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15.log25202.0 step2 runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step2_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log10021.0 step2 runTheMatrix-results/10021.0_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step2_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log10024.0 step2 runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step2_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log
I found errors in the following addon tests: cmsRun /cvmfs/cms-ib.cern.ch/nweek-02457/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_0_X_2017-02-01-2300/src/HLTrigger/Configuration/test/OnLine_HLT_PRef.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Thu Feb 2 21:04:08 2017-date Thu Feb 2 21:01:57 2017 s - exit: 18688 |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
On 2/2/17 9:03 PM, Matti Kortelainen wrote:
Thanks @ebrondol <https://github.com/ebrondol>. But I'm afraid this is
not enough because LowPtClusterShapeSeedComparitor appears to be used in
HLT. But we'll see if the tests succeed or fail.
I had a similar feeling as well about HLT, now confirmed.
If it's not feasible to get this in through fillDescriptions,
make a customise for HLT.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17393 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcborwDaf37iJTOfBiNHI1cELfo22Dks5rYjZ0gaJpZM4L1W7I>.
|
IMHO not feasible. It would require transforming all SeedComparitors to ED/ESProducers, and I'd much rather revamp the whole SeedComparitor infrastructure instead (it's messy and confusing to understand). |
Ok, I did not know that. Was I suppose to close this PR? |
You can update this PR, no need to close, if the fix is reasonably clear
…On Feb 4, 2017 4:18 AM, "ebrondol" ***@***.***> wrote:
Ok, I did not know that. Was I suppose to close this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17393 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbmwHVSii0Q4Cu-XN1bjUhAN6cHwSks5rZEJbgaJpZM4L1W7I>
.
|
@slava77 working on that. |
Pull request #17393 was updated. @perrotta, @cmsbuild, @silviodonato, @cvuosalo, @fwyzard, @Martin-Grunewald, @slava77, @davidlange6 can you please check and sign again. |
@cmsbuild please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
+1 |
for producer in producers_by_type(process,"PixelTripletHLTEDProducer"): | ||
if hasattr(producer,'SeedComparitorPSet'): | ||
if (producer.SeedComparitorPSet.ComponentName.value() == 'LowPtClusterShapeSeedComparitor'): | ||
producer.SeedComparitorPSet.clusterShapeHitFilter = cms.string('ClusterShapeHitFilter') |
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.
@ebrondol , in this way you always overwrite clusterShapeHitFilter in all 'LowPtClusterShapeSeedComparitor' component.
In other word we cannot control the clusterShapeHitFilter value from the HLT configuration, because it will be always overwritten by 'ClusterShapeHitFilter'. Did you want this? Did you forget to add if not hasattr(producer.SeedComparitorPSet,'clusterShapeHitFilter')
? Thanks for the check, Silvio.
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.
Hi, sorry for the delay. Yes, you are completely right. I will address it asap. Thanks for spotting the issue.
After a private chat with @makortel, we decided to set the pixel cluster shape filter in the 'LowPtClusterShapeSeedComparitor' configurable. The 'LowPtClusterShapeSeedComparitor' is used for all pixel use cases and the cluster shape filtering name was set hard-coded to 'ClusterShapeHitFilter'.
This change increases the flexibility of the code and the relationship between the two is also showing up in edmConfigDump, while it was not clear before.
No changes are expected in any phases.