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

MuonHLT IterL3 reconstruction: bug fix to avoid running twice on the same L1 #18957

Merged
merged 2 commits into from May 27, 2017

Conversation

folguera
Copy link
Contributor

This prevents to run twice on an already used L1, performance is not affected, but timing would be significantly reduced. This bug was introduced by PR #18830. Do we need a back port to 91X?

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @folguera (Santiago Folgueras) for master.

It involves the following packages:

HLTrigger/Muon

@Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @abbiendi, @jhgoh, @Martin-Grunewald, @calderona, @HuguesBrun, @trocino this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor

fwyzard commented May 26, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 26, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20130/console Started: 2017/05/26 10:54

@fwyzard
Copy link
Contributor

fwyzard commented May 26, 2017

thanks Santiago, there is no need for a 91x backport

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@@ -53,7 +53,6 @@ HLTL1MuonNoL2Selector::fillDescriptions(edm::ConfigurationDescriptions& descript
edm::ParameterSetDescription desc;
desc.add<edm::InputTag>("InputObjects",edm::InputTag(""));
desc.add<edm::InputTag>("L2CandTag",edm::InputTag("hltL2MuonCandidates"));
desc.add<edm::InputTag>("L1CandTag",edm::InputTag(""));
Copy link
Contributor Author

@folguera folguera May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've removed this because it is not used anywhere in the code, but the removal of this line require parsing the code to ConfDB as the ConfDB version is still asking for this parameter. How do we want to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? The jenkins PR tests pass, so the module is not yet used.
So the PR can get integrated, and we can parse it into ConfDB later.

@fwyzard
Copy link
Contributor

fwyzard commented May 26, 2017

Options:

  • write a customisation function in HLTrigger/Configuration/python/customizeHLTforCMSSW.py
  • change it to
  // # OBSOLETE - these parameters are ignored, they are left only not to break old configurations
  // they will not be printed in the generated cfi.py file
  desc.addOptionalNode(edm::ParameterDescription<edm::InputTag>("L1CandTag", "",  false), false)->setComment("This parameter is obsolete and will be ignored.");
  • wait for STORM to parse this PR in ConfDB

@Martin-Grunewald @silviodonato what do you prefer ?

@Martin-Grunewald
Copy link
Contributor

2 or 3 would be fine, 1 is a bit overkill.... as the module is not yet used.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18957/20130/summary.html

Comparison Summary:

  • You potentially added 38 lines to the logs
  • Reco comparison results: 3401 differences found in the comparisons
  • DQMHistoTests: Total files compared: 24
  • DQMHistoTests: Total histograms compared: 1833626
  • DQMHistoTests: Total failures: 33458
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1799988
  • DQMHistoTests: Total skipped: 180
  • DQMHistoTests: Total Missing objects: 0
  • Checked 98 log files, 14 edm output root files, 24 DQM output files

@cmsbuild
Copy link
Contributor

Pull request #18957 was updated. @Martin-Grunewald, @silviodonato, @cmsbuild, @fwyzard, @davidlange6 can you please check and sign again.

@folguera
Copy link
Contributor Author

@fwyzard, @Martin-Grunewald I followed option 2 from Andrea's suggestion.

@fwyzard
Copy link
Contributor

fwyzard commented May 26, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 26, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20135/console Started: 2017/05/26 15:20

@fwyzard
Copy link
Contributor

fwyzard commented May 26, 2017

@folguera I think you are officially the second person in CMS to make use of this possibility :-)

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18957/20135/summary.html

Comparison Summary:

  • You potentially added 109 lines to the logs
  • Reco comparison results: 3415 differences found in the comparisons
  • DQMHistoTests: Total files compared: 24
  • DQMHistoTests: Total histograms compared: 1833626
  • DQMHistoTests: Total failures: 47794
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1785652
  • DQMHistoTests: Total skipped: 180
  • DQMHistoTests: Total Missing objects: 0
  • Checked 98 log files, 14 edm output root files, 24 DQM output files

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit cc64914 into cms-sw:master May 27, 2017
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