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.

Sign up for GitHub

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

Jump to bottom

[L1T] Add PF developments for L1T phase-2 (new) #38230

Merged
merged 4 commits into from Jun 9, 2022
Merged

[L1T] Add PF developments for L1T phase-2 (new) #38230

merged 4 commits into from Jun 9, 2022

Conversation

cecilecaillol
Copy link
Contributor

PR description:

Same as #38045, #38047, #37536

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."

This version of the PR has the reorganization of the code to have a structure more CMS friendly, and has the previous commits squashed into one for clarity. It also fixes the DQM problem mentionned in the previous PRs.

@cmsbuild cmsbuild added this to the CMSSW_12_5_X milestone Jun 3, 2022
@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38230/30381

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

A new Pull Request was created by @cecilecaillol for master.

It involves the following packages:

  • DQMOffline/L1Trigger (dqm, l1)
  • DataFormats/L1TCorrelator (upgrade, l1)
  • DataFormats/L1TParticleFlow (upgrade, l1)
  • DataFormats/L1Trigger (l1)
  • L1Trigger/Configuration (l1)
  • L1Trigger/Phase2L1ParticleFlow (upgrade, l1)

@rekovic, @epalencia, @emanueleusai, @ahmad3213, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @pmandrik, @micsucmed, @cecilecaillol, @rvenditti can you please review it and eventually sign? Thanks.
@kreczko, @rovere, @eyigitba, @Martin-Grunewald, @missirol, @dinyar, @thomreis, @trtomei, @beaucero, @rociovilar this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5dffa5/25249/summary.html
COMMIT: 699c10a
CMSSW: CMSSW_12_5_X_2022-06-03-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38230/25249/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 03-Jun-2022 18:28:30 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'dqmoffline_7_step'
   [2] Calling method for module L1TPhase2CorrelatorOffline/'l1tPhase2CorrelatorOfflineDQM'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for a container with elements of type: reco::Candidate
Looking for module label: l1ctLayer1
Looking for productInstanceName: PF

   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------

----- Begin Fatal Exception 03-Jun-2022 18:29:04 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'dqmoffline_7_step'
   [2] Calling method for module L1TPhase2CorrelatorOffline/'l1tPhase2CorrelatorOfflineDQM'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for a container with elements of type: reco::Candidate
Looking for module label: l1ctLayer1
Looking for productInstanceName: PF

   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------

----- Begin Fatal Exception 03-Jun-2022 18:29:16 CEST-----------------------
An exception of category 'ProductNotFound' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 1 stream: 0
   [1] Running path 'dqmoffline_7_step'
   [2] Calling method for module L1TPhase2CorrelatorOffline/'l1tPhase2CorrelatorOfflineDQM'
Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for a container with elements of type: reco::Candidate
Looking for module label: l1ctLayer1
Looking for productInstanceName: PF

   Additional Info:
      [a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------
Expand to see more relval errors ...

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38230/30388

@cecilecaillol
Copy link
Contributor Author

@AdrianoDee Sorry this is still the same big PR :) But I think this time it can be merged! Can you please sign again?

@cecilecaillol
Copy link
Contributor Author

@AdrianoDee @srimanob Could you please sign? This is identical to what you had signed off before (just had to reopen a new PR for technical reasons). We need to make a few other PRs after this one and it is getting tight for our phase-2 MC production

@AdrianoDee
Copy link
Contributor

AdrianoDee commented Jun 8, 2022 •
edited

Hi @cecilecaillol, just two minor comments about something I may had overlooked:

  • there is a bunch of changes in the DQM plots (mainly in the L1T Correlator) for few workflows. I imagine these are mostly expected. Just checking since not all of them are "obvious" improvements. Just an example from 39634.999

image

  • there are some existsAs methods that could be cleaned in a further PR (that is something I have understood will anyway come given some of this code need to be cleaned or deleted).

Anyway this PR is good to go for me.

@cecilecaillol
Copy link
Contributor Author

Hi @cecilecaillol, just two minor comments about something I may had overlooked:

* there is a bunch of changes in the DQM plots (mainly in the L1T Correlator) for few workflows. I imagine these are mostly expected. Just checking since not all of them are "obvious" improvements. Just an example from `39634.999`

image

* there are some `existsAs` methods that could be cleaned in a further PR (that is something I have understood will anyway come given some of this code need to be cleaned or deleted).

Anyway this PR is good to go for me.

Thanks @AdrianoDee ! We will follow-up on both points in next PRs (we have a few DQM changes to make, and we will clean some files and check if there are some existsAs left)

@cecilecaillol
Copy link
Contributor Author

@AdrianoDee If the PR is good to go from your side, could you please add the upgrade signature?

@AdrianoDee
Copy link
Contributor

Sorry if I insist but I didn't understand if the discrepancies are in someway expected or not. I understood that you intend to fix/change DQM modules.

I just wouldn't like to overlook something that would be difficult to spot if it goes in the release regardless of possible future fixes (:

@cecilecaillol
Copy link
Contributor Author

@drankincms Can you please comment more about the DQM plots to answer Adriano's question above? The switch from l1pfCandidates to l1ctLayer1 you mentioned a few days ago will be part of a next PR.

@drankincms
Copy link
Contributor

@cecilecaillol @AdrianoDee Thanks for flagging. These all look expected to me. The PF results should be and are unchanged, and there should be some PUPPI differences as we see. This brings them more in line with what I would have expected.

@AdrianoDee
Copy link
Contributor

@cecilecaillol @AdrianoDee Thanks for flagging. These all look expected to me. The PF results should be and are unchanged, and there should be some PUPPI differences as we see. This brings them more in line with what I would have expected.

@drankincms & @cecilecaillol thanks for the clarifications.

@AdrianoDee
Copy link
Contributor

+upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). 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)

@cecilecaillol
Copy link
Contributor Author

@perrotta, @dpiparo, @qliphy Can this be merged please? We need this PR backported to 12_4 for our MC production, as well as other PRs depending on this one, and it is getting tight for us to get everything ready before 12_4 closes.

@perrotta
Copy link
Contributor

perrotta commented Jun 9, 2022

@drankincms dqm comparisons for Puppi plots are the same as for previous versions of this PR, e.g.:
image

In #38047 (comment) you were claiming that "It looks like the switch from l1pfCandidates to l1ctLayer1 hasn't been propagated here if I am reading things correctly. This change is in the l1t-offline branch, @cecilecaillol could you add that change to this PR to fix perhaps?"

As far as I understand this change was not propagated here, and these plots witness an error in the implementation. But you comment of yesterday in #38230 (comment) actually contradicts it.

Maybe I am missing some intermediate info (sorry, but I'm curently in vacation and I can hardly follow the whole thread) but could you please make clear:

  • Is that an error, or not?
  • If it is an error, and it can be fixed by adding some already prepared code, why don't you do it now? As @AdrianoDee also pointed out, if some wrong distribution remains in the reference plots, it will be more difficult to fix them in the future.

For what concerns all other comments of mine I made in #38047 (review), I see that they were all taken into accont, even if some further cleaning has to be expected. I would have preferred cleaning in this PR already, thus avoiding having to review and maintain some code which is already known to be obsolete, but if it is easier for you doing it in two steps, please proceed as you believe it is more practical: ok for me to let them in for the moment.

@cecilecaillol
Copy link
Contributor Author

@perrotta the switch from l1pfCandidates to l1ctLayer1 hasn't been propagated, which is why this DQM comparison is unchanged. I tried making the change in this PR, but got errors in the unit tests, and given the size of the PR it takes forever for me to check out the code, compile, make changes, validate, etc. When this big PR is merged I will follow up immediately with smaller PRs, including this DQM change, which will be much easier to validate when I am modifying only one file or two. Similarly, the cleaning of obsolete files was not so straightforward because of dependencies in some tests, and that will be a separate standalone PR in the coming days.

@drankincms
Copy link
Contributor

Maybe just to add: sorry for the confusion here, perhaps my use of "expected" was a bit ambiguous. I meant that indeed I think it is expected that these look as they do given that the DQM has not been updated to take the new correct emulation.

@perrotta
Copy link
Contributor

perrotta commented Jun 9, 2022

+1

  • Meant to be an intermediate step.
  • Further fixes and cleanings expected in subsequent PRs

@cmsbuild cmsbuild merged commit dc56965 into cms-sw:master Jun 9, 2022
@cecilecaillol cecilecaillol mentioned this pull request Jun 13, 2022
Closed
@perrotta perrotta mentioned this pull request Jun 17, 2022
Closed
@cecilecaillol cecilecaillol mentioned this pull request Jun 21, 2022
Merged
@kskovpen kskovpen mentioned this pull request Jul 26, 2022
Closed
@cecilecaillol cecilecaillol deleted the l1t-squashedpfv2 branch September 23, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved fully-signed l1-approved orp-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_12_5_X
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cecilecaillol @cmsbuild @AdrianoDee @drankincms @perrotta