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.

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

Fix L1GtUtilsHelper's search for data products to consume #38900

Merged
merged 1 commit into from Aug 3, 2022

Conversation

Dr15Jones
Copy link
Contributor

PR description:

The previous algorithm incorrectly kept the first data product that matched its type. The intent was to try to find the preferred module labels and if it can't then keep a data product if there is only one choice. The algorithm now does that behavior.

PR validation:

Running #38603 showed that the histograms generated by using this as a filter were filled differently based on the order in which this code saw data products. Now with or without #38603 the same data products are chosen.

The previous algorithm incorrectly kept the first data product
that matched its type. The intent was to try to find the preferred
module labels and if it can't then keep a data product if there is
only one choice. The algorithm now does that behavior.
@Dr15Jones
Copy link
Contributor Author

Although I kept the case where the configuration has an ambiguity in which data products to retrieve just a LogError message, it would be better to change it to an exception.

The best change would be to do away with the attempt to automatically pick the data product and instead require the InputTag to be specified in the configuration.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38900/31333

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • L1Trigger/GlobalTriggerAnalyzer (l1)

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

cms-bot commands are listed here

@makortel
Copy link
Contributor

The

void l1t::L1TGlobalUtilHelper::operator()(edm::BranchDescription const& branchDescription) {
has very similar structure, maybe it would need to be fixed as well?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a72e90/26536/summary.html
COMMIT: d6d0b96
CMSSW: CMSSW_12_5_X_2022-07-29-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38900/26536/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 5339 lines to the logs
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3668050
  • DQMHistoTests: Total failures: 18
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3668010
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@Dr15Jones
Copy link
Contributor Author

The change in histograms for WF 4.53 are expected. The base line histograms were empty because the old algorithm incorrectly chose data products that were not in the event. The new algorithm does what the comments for the old algorithm said was supposed to be done and now finds a 'preferred' data product which is in the event and therefore the new algorithm fills the histograms.

The increase to the log is also expected as the code now reports the cases where the configuration should be changed to avoid unnecessary data access.

@makortel
Copy link
Contributor

makortel commented Aug 2, 2022

@cms-sw/l1-l2 Would you be able to review this PR quickly? We have three framework PRs (#38603, #38806, #38872) that show comparison differences because of this problem. Thanks!

@epalencia
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Aug 3, 2022

+1

@cmsbuild cmsbuild merged commit 186424c into cms-sw:master Aug 3, 2022
@Dr15Jones Dr15Jones deleted the fixL1GtUtilsHelper branch September 2, 2022 16:01
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.

None yet

5 participants