Skip to content

Conversation

@EmyrClement
Copy link
Contributor

PR description:

Add missing seeded cone jet collection to event, which are produced using the "extended track" collection.

These are required as #40670 adds an edm::ValueMap<float> to the event and produces the corresponding reference objects, but the reference objects are not stored in the output. Therefore these changes are nominally on top of #40670.

For completeness, we also add the intermediate inputs that are used to produce these new seeded cone jet collection i.e. the puppi candidates produced using the "extended tracks", as is done for the existing seeded cone jet collection (those produced using the regular tracks).

The corresponding PR to the L1T integration branch is here.

PR validation:

Ran standard cmsDriver command from runTheMatrix. By itself, this PR doesn't change anything. On top of #40670, I see the new objects are present in the output root file.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41258/35005

  • This PR adds an extra 12KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 3, 2023

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

It involves the following packages:

  • L1Trigger/Configuration (l1)

@epalencia, @cmsbuild, @cecilecaillol, @aloeliger can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@epalencia
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 3, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0c35b6/31760/summary.html
COMMIT: d3f00bd
CMSSW: CMSSW_13_1_X_2023-04-03-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41258/31760/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 12 lines to the logs
  • Reco comparison results: 10 differences found in the comparisons
  • DQMHistoTests: Total files compared: 47
  • DQMHistoTests: Total histograms compared: 3364817
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3364783
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 46 files compared)
  • Checked 202 log files, 155 edm output root files, 47 DQM output files
  • TriggerResults: no differences found

@aloeliger
Copy link
Contributor

Corresponds to cms-l1t-offline#1090

@aloeliger
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2023

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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2023

@EmyrClement is there a reason why this (technically) simple addition to the L1Trigger_EventContent is made in a separate PR with respect to #41260, where the same event content is also updated? Since they refer to the same correlated additions, I would find natural that this is added as an additional commit to #41260, so that they can be tested and merged all at once. Please let me know if I am missing anything.

In any case, when addditional collections are added to an event content, is good habit to report in the corresponding PR the additional event size which is generated by them. Please do so either here or in #41260.

@aloeliger
Copy link
Contributor

@perrotta The original owner of the correlator PR was having a difficult time getting the PR to work properly, and so asked for no more additions to it. Since the decision was made to make this PR separately, I have taken the correlator PR over. I have no objection to adding this PR onto it if this PR is too trivial.

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2023

@aloeliger I see you are having hard times with the other PR :-)

These two must go together. Let say, mine is not a strict request, but I'm convinced it can be easier for you to include this simple commit in the other PR (whatever its number will be). As such you won't risk to have to rebase it once again if this one is merged; you can have a better control on the overall additional event content and its size (which should be reported in the PR descriptions at some time).

Overall, I think that it will be easier for you to include this commit in the other PR. But if you believe otherwise, please keep them separated. We will put this one on hold till the other one is merged, then, in order not to mess it up even further.

@perrotta
Copy link
Contributor

perrotta commented Apr 5, 2023

hold
(It has to wait that the L1T phase2 correlator PR is merged first)

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2023

Pull request has been put on hold by @perrotta
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@aloeliger
Copy link
Contributor

@EmyrClement Please open a PR to the new squashed branch containing the correlator PR info in #41279

@EmyrClement
Copy link
Contributor Author

Closing, as now included in #41279 as discussed.

@EmyrClement EmyrClement closed this Apr 5, 2023
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.

5 participants