PFEGammaProducer to global producer #29375

guitargeek · 2020-04-01T21:28:03Z

PR description:

The PF egamma code is still a big chunk of hard-to-understand code; this PR does not really do much about that but it goes in the direction by making the PFEGammaProducer a global module and removing an unused configuration parameter that would not have an effect even if enabled (useVerticesForNeutral, the dummy vertex was overwritten by the real reconstructed vertex anyways). Furthermore, the producer got modernized with put tokens and the includes in the PFProducer packages were cleaned by not including forward header files in .cc sources anymore (which required a few little changes in the DataFormats, as there were some header files which didn't include their own forward header which should optimally be the case).

PR validation:

CMSSW compiles and matrix tests pass.

if this PR is a backport please specify the original PR and why you need to backport that PR:

No backport intended.

cmsbuild · 2020-04-01T21:28:29Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-04-01T21:32:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29375/14480

This PR adds an extra 104KB to repository

cmsbuild · 2020-04-01T21:33:13Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

DataFormats/EgammaReco
DataFormats/ParticleFlowReco
DataFormats/VertexReco
RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @Sam-Harper, @makortel, @jainshilpi, @afiqaize, @rovere, @lgray, @sobhatta, @hatakeyamak, @VinInn, @varuns23, @seemasharmafnal, @cbernet this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

slava77 · 2020-04-01T21:40:59Z

@cmsbuild please test

cmsbuild · 2020-04-01T21:41:18Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5494/console Started: 2020/04/01 23:44

cmsbuild · 2020-04-02T03:22:50Z

-1

Tested at: 7216d10

CMSSW: CMSSW_11_1_X_2020-04-01-1100
SCRAM_ARCH: slc7_amd64_gcc820

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

Make some HLT modules into global modules #29301 @Dr15Jones: Make some HLT modules into global modules
Remove unneeded dependencies from plugins, test and bin in Reco subsystems #29295 @guitargeek: Remove unneeded dependencies from plugins, test and bin in Reco subsystems
HLT migration to 11_1_0_pre5 and update of TSG tests #29357 @cms-tsg-storm: HLT migration to 11_1_0_pre5 and update of TSG tests

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5494/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5494/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5494/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test testTauEmbeddingProducers had ERRORS

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

Make some HLT modules into global modules #29301 @Dr15Jones: Make some HLT modules into global modules
Remove unneeded dependencies from plugins, test and bin in Reco subsystems #29295 @guitargeek: Remove unneeded dependencies from plugins, test and bin in Reco subsystems
HLT migration to 11_1_0_pre5 and update of TSG tests #29357 @cms-tsg-storm: HLT migration to 11_1_0_pre5 and update of TSG tests

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5494/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5494/git-merge-result

cmsbuild · 2020-04-02T03:22:53Z

Comparison job queued.

cmsbuild · 2020-04-02T08:38:14Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 20 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2692102
DQMHistoTests: Total failures: 53
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2691730
DQMHistoTests: Total skipped: 319
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

perrotta · 2020-04-02T14:15:49Z

@cmsbuild please test

cmsbuild · 2020-04-02T14:16:12Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5502/console Started: 2020/04/02 16:17

cmsbuild · 2020-04-03T14:13:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5522/console Started: 2020/04/04 03:48

cmsbuild · 2020-04-04T03:31:27Z

+1
Tested at: a8fe68a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9fe532/5522/summary.html
CMSSW: CMSSW_11_1_X_2020-04-03-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-04-04T03:31:30Z

Comparison job queued.

cmsbuild · 2020-04-04T16:20:47Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 49 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2692110
DQMHistoTests: Total failures: 45
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2691746
DQMHistoTests: Total skipped: 319
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

perrotta · 2020-04-05T09:31:31Z

@silviodonato since a few days it is hard to compare the reco outputs of the automatic tests because there are a lot of messages like

%MSG
[2020-04-04 02:11:46.060760 +0200][Error  ][PostMaster        ] [eoscms.cern.ch:1094 #0] Forcing error on disconnect: [ERROR] Operation interrupted.
%MSG-e ExcessiveTime:  SiPixelClusterProducer:siPixelClustersPreSplitting  04-Apr-2020 02:29:51 CEST Run: 326479 Event: 1394020
ExcessiveTime: Module used 1837.07 seconds of time which exceeds the error threshold configured in the Timing Service of 600 seconds.
%MSG

in the logs. And, according to some random inspection I made, all those ExcessiveTime error's are in the baseline logs, and not in the PR's logs.

Is there an explanation for this? Can we expect a fix is coming, so that we can more easily compare the outputs?

perrotta · 2020-04-06T07:52:59Z

Please @guitargeek clean up the PR description in the part dealing with the forward header files, as these changes were reverted in this PR

perrotta · 2020-04-06T07:56:15Z

+1

Technical changes, leading to make the PFEGammaProducer a global producer, some cleaning, and removal of the useVerticesForNeutral parameter and its usage in the code (as it was actually a deadly and useless assignment, as pointed out in the PR description)
Jenkins tests pass and show no differences, as it should, besides the spurious ones described
above

cmsbuild · 2020-04-06T07:56:41Z

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

silviodonato · 2020-04-06T08:18:32Z

+1

makortel · 2020-04-06T13:26:27Z

@silviodonato since a few days it is hard to compare the reco outputs of the automatic tests because there are a lot of messages like
%MSG
[2020-04-04 02:11:46.060760 +0200][Error  ][PostMaster        ] [eoscms.cern.ch:1094 #0] Forcing error on disconnect: [ERROR] Operation interrupted.
%MSG-e ExcessiveTime:  SiPixelClusterProducer:siPixelClustersPreSplitting  04-Apr-2020 02:29:51 CEST Run: 326479 Event: 1394020
ExcessiveTime: Module used 1837.07 seconds of time which exceeds the error threshold configured in the Timing Service of 600 seconds.
%MSG
in the logs. And, according to some random inspection I made, all those ExcessiveTime error's are in the baseline logs, and not in the PR's logs.

Is there an explanation for this? Can we expect a fix is coming, so that we can more easily compare the outputs?

@perrotta If this is a persistent problem, could you open an issue about it?

slava77 · 2020-04-06T14:38:50Z

@perrotta If this is a persistent problem, could you open an issue about it?

I've seen it in another case about a week ago (but I can't find that example to be more precise)

cmsbuild added this to the CMSSW_11_1_X milestone Apr 1, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 1, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 1, 2020

cmsbuild mentioned this pull request Apr 1, 2020

Restrict timestamps in PFCandidates to reliable cases #29374

Merged

cmsbuild added tests-started and removed tests-pending labels Apr 1, 2020

cmsbuild added tests-rejected and removed tests-started labels Apr 2, 2020

cmsbuild added comparison-available and removed comparison-pending labels Apr 2, 2020

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-rejected labels Apr 2, 2020

cmsbuild added tests-started and removed tests-pending labels Apr 3, 2020

cmsbuild added tests-approved and removed tests-started labels Apr 4, 2020

cmsbuild added comparison-available and removed comparison-pending labels Apr 4, 2020

perrotta mentioned this pull request Apr 6, 2020

[HGCAL] TICL in reconstruction #29081

Merged

guitargeek changed the title ~~PFEGammaProducer to global producer and other changes~~ PFEGammaProducer to global producer Apr 6, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Apr 6, 2020

cmsbuild added orp-approved and removed orp-pending labels Apr 6, 2020

cmsbuild merged commit 511d715 into cms-sw:master Apr 6, 2020

guitargeek deleted the PFEGammaProducer_1 branch April 6, 2020 08:20

perrotta mentioned this pull request Apr 6, 2020

Remove unused SeedFilter class in EgammaElectronAlgos to simplify ElectronSeedProducer #29304

Merged

silviodonato mentioned this pull request Apr 6, 2020

"ExcessiveTime: SiPixelClusterProducer:siPixelClustersPreSplitting" causes fake reco comparison failures #29398

Closed

guitargeek mentioned this pull request Apr 7, 2020

Don't include forward headers in cc files in RecoParticleFlow/PFProducer package #29410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PFEGammaProducer to global producer #29375

PFEGammaProducer to global producer #29375

guitargeek commented Apr 1, 2020

cmsbuild commented Apr 1, 2020

cmsbuild commented Apr 1, 2020

cmsbuild commented Apr 1, 2020

slava77 commented Apr 1, 2020

cmsbuild commented Apr 1, 2020 •

edited

cmsbuild commented Apr 2, 2020

cmsbuild commented Apr 2, 2020

cmsbuild commented Apr 2, 2020

perrotta commented Apr 2, 2020

cmsbuild commented Apr 2, 2020 •

edited

cmsbuild commented Apr 3, 2020 •

edited

cmsbuild commented Apr 4, 2020

cmsbuild commented Apr 4, 2020

cmsbuild commented Apr 4, 2020

perrotta commented Apr 5, 2020

perrotta commented Apr 6, 2020

perrotta commented Apr 6, 2020

cmsbuild commented Apr 6, 2020

silviodonato commented Apr 6, 2020

makortel commented Apr 6, 2020

slava77 commented Apr 6, 2020

PFEGammaProducer to global producer #29375

PFEGammaProducer to global producer #29375

Conversation

guitargeek commented Apr 1, 2020

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Apr 1, 2020

cmsbuild commented Apr 1, 2020

cmsbuild commented Apr 1, 2020

slava77 commented Apr 1, 2020

cmsbuild commented Apr 1, 2020 • edited

cmsbuild commented Apr 2, 2020

cmsbuild commented Apr 2, 2020

cmsbuild commented Apr 2, 2020

perrotta commented Apr 2, 2020

cmsbuild commented Apr 2, 2020 • edited

cmsbuild commented Apr 3, 2020 • edited

cmsbuild commented Apr 4, 2020

cmsbuild commented Apr 4, 2020

cmsbuild commented Apr 4, 2020

perrotta commented Apr 5, 2020

perrotta commented Apr 6, 2020

perrotta commented Apr 6, 2020

cmsbuild commented Apr 6, 2020

silviodonato commented Apr 6, 2020

makortel commented Apr 6, 2020

slava77 commented Apr 6, 2020

cmsbuild commented Apr 1, 2020 •

edited

cmsbuild commented Apr 2, 2020 •

edited

cmsbuild commented Apr 3, 2020 •

edited