Add PF developments for L1T phase-2 #38047
Conversation
|
A new Pull Request was created by @perrotta (Andrea Perrotta) for master. It involves the following packages:
@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
With only one commit, there are no files with invalid status in git |
|
please test |
|
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test SiStripDAQ_O2O_test had ERRORS Comparison SummarySummary:
|
|
I am fine with merging this PR and closing the other ones. I dont know why the unit tests fail, though. |
|
+l1 |
|
+upgrade
|
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
There was a problem hiding this comment.
It is extremely hard to debug such large amount of code, and for sure several bugs and issues will be found only when tested on some large poduction. I would suggest however to have at least a look at the Static Analyzer report of the automatic bot tests, as it quite often points to some actual flaw, that can be identified thanks to it, and for which a fix can be provided since now. Most of the comments above derives from the inspection of the SA output for this PR
| ptsums.push_back(std::pair<float, float>(vtx.pt(), vtx.z0())); | ||
| if (ptsum == 0 || vtx.pt() > ptsum) { | ||
| ptsum = vtx.pt(); | ||
| z0 = vtx.z0(); |
There was a problem hiding this comment.
This z0 is not needed here (as pointed out by the Static Analyzer)
| continue; | ||
| float pVal = h_dz->GetBinContent(b); | ||
| if (pMax < pVal || lBin[vtx] == -1) { | ||
| pVal = pMax; |
There was a problem hiding this comment.
Shouldn't this be
| pVal = pMax; | |
| pMax = pVal; |
(as suggested by the Static Analyzer)?
There was a problem hiding this comment.
yes. that said, this entire file is to be deleted in a new PR.
There was a problem hiding this comment.
yes. that said, this entire file is to be deleted in a new PR.
Why not now, then?
There was a problem hiding this comment.
In principle, now is fine for me as well, but it's up to @drankincms and @cecilecaillol to work this out, not me
| int LowerSize = PowerOf2LessThan(InSize); //-- LowerSize >= Size / 2 | ||
| int UpperSize = InSize - LowerSize; //-- UpperSize < LowerSiz | ||
|
|
||
| if (LowerSize < UpperSize) |
There was a problem hiding this comment.
Shouldn't you gracefully return here, maybe also adding in the check that UpperSize is above 0?
(As suggested by the Static Analyzer)
There was a problem hiding this comment.
no, I think here we should ungracefully stop because this is not something that is ever expected to happen in any valid circumstance. we could add an assert.
|
|
||
| template <typename T> | ||
| void bitonicSort(const T in[], int Start, int InSize, T out[], int OutSize, bool dir) { | ||
| if (InSize <= 1) // copy in-> out and exit |
There was a problem hiding this comment.
Could a check that OutSize>0 help here?
(As suggested by the Static Analyzer)
There was a problem hiding this comment.
it doesn't make sense to ever call this function with outSize <= 0 (why bother sorting N elements just to throw away all of them), so in principle we could add an assert.
| } | ||
| switch (algo) { | ||
| case VertexAlgo::External: { | ||
| int lBin[nVtx_]; |
There was a problem hiding this comment.
I think all elements in this vector should be initialized to "-1" if the logic at L126 can work
(As suggested by the Static Analyzer)
There was a problem hiding this comment.
No idea, this piece of code was added by @violatingcp so he may know better, but anway we should remove this entire file in a new PR because it's obsolete
| debug = cms.bool(False) | ||
| ) | ||
|
|
||
| L1SeedConePFJetEmulatorProducer = L1SeedConePFJetProducer.clone(HW = cms.bool(True)) |
There was a problem hiding this comment.
| L1SeedConePFJetEmulatorProducer = L1SeedConePFJetProducer.clone(HW = cms.bool(True)) | |
| L1SeedConePFJetEmulatorProducer = L1SeedConePFJetProducer.clone(HW = True) |
| @@ -99,6 +38,8 @@ | |||
| vtxAlgo = "external", | |||
| vtxFormat = cms.string("TkPrimaryVertex"), | |||
There was a problem hiding this comment.
Given the complexity of these configs (and the lack of fillDescriptions methods in most of the code in this PR) it would be safer to remove all types from the cloned parameters) I understand that this could also go in a further cleanup PR, once this is merged
There was a problem hiding this comment.
yes, in particular this l1ParticleFlow_cff file is to be deleted.
| @@ -145,7 +89,7 @@ | |||
| pfAlgo = "PFAlgo2HGC", | |||
| # inputs | |||
| tracks = cms.InputTag('pfTracksFromL1TracksHGCal'), | |||
| @@ -228,6 +175,8 @@ | |||
| vtxAlgo = "external", | |||
| vtxFormat = cms.string("TkPrimaryVertex"), | |||
| from L1Trigger.Phase2L1ParticleFlow.DeregionizerProducer_cfi import DeregionizerProducer as l1ctLayer2Deregionizer | ||
| scPFL1PF = L1SeedConePFJetProducer.clone(L1PFObjects = 'l1ctLayer1:PF') | ||
| scPFL1Puppi = L1SeedConePFJetProducer.clone() | ||
| scPFL1PuppiEmulator = L1SeedConePFJetEmulatorProducer.clone(L1PFObject = cms.InputTag('l1ctLayer2Deregionizer', 'Puppi')) |
There was a problem hiding this comment.
| scPFL1PuppiEmulator = L1SeedConePFJetEmulatorProducer.clone(L1PFObject = cms.InputTag('l1ctLayer2Deregionizer', 'Puppi')) | |
| scPFL1PuppiEmulator = L1SeedConePFJetEmulatorProducer.clone(L1PFObject = 'l1ctLayer2Deregionizer:Puppi') |
|
@cecilecaillol @gpetruc please have a look at the comments above @smuzaffar I cannot find the git instruction to let @cecilecaillol pushing commits to this PR: does it exist any such possibility? |
|
@cecilecaillol about the differences reported in the L1T Phase2 DQM, are they all understood? (They are the same here and in the previous version of this PR). For example (from wf 39434): Is it correct that these plots are now emppty? or much less populated when including all categories? "Non puppi" plots show just some fluctuation in the differences, which is probably what should be expected: but please give a glance also to them. |
@perrotta , @cecilecaillol can not push to your branch. I think only users with push rights to cms-sw/cmssw can do it ( see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork ). I think @cecilecaillol can open a PR with changes for perrotta/cmssw branch l1t-squashedpf and then you can |
Thank you @smuzaffar @cecilecaillol as you can see from the messages above. still some work is needed for this PR, at least some cleaning, as also suggested by @gpetruc |
|
ping @cecilecaillol on above comments |
@gpetruc Can you check if the DQM plots are as expected? |
I don't know anything about them, they were added by @drankincms |
It looks like the switch from |
@drankincms Thanks! Can you point me to the code or corresponding PR in cms-l1t-offline? |
|
Sure! This is the PR: cms-l1t-offline@1b2ffd5 |
|
Closing in favour of #38178 |
PR description:
This is the same as #38045 from @cecilecaillol (originally in #37536) with all commits squashed into 1, with the idea of getting rid of the huge amount of files with invalid states as listed in #38045 (comment)
Original description: "This is another PR to port all L1T Phase-2 developments to cms-sw. This PR targets the particle flow code. It includes several PRs made locally, the details of which can be found in the twiki https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideL1TPhase2Instructions and in the cms-l1t-offline branch.
The code is equivalent to the old PR #37452 but has been reorganized to have a structure more CMS-friendly."
Of course, I'll let Cecile decide which version to eventually merge in CMSSW, or whether to import this single commit into a new PR of her, in case some further development is needed.