Remove redundant product validity check from ConversionTrackProducer and modernize it #30082

makortel · 2020-06-02T18:41:44Z

PR description:

This PR is part of #30074 even though it itself does not fix any workflow. The bare "throw" will lead to abort (*) because there is no exception to be thrown. On the other hand, the beamSpotHandle->position() will throw an exception if the product is missing (with much clearer message), so this PR proposes just remove the check.

(*) https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/raw/slc7_amd64_gcc820/CMSSW_11_2_X_2020-06-01-2300/pyRelValMatrixLogs/run/135.9_ZMM_13+ZMMFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZMM_13+ZMMFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

In addition this PR modernizes ConversionTrackProducer by

Constructing BeamSpot position as math::XYZVector once
Using edm::Event::get() instead of going via edm::Handle
Consuming trajectory maps only if useTrajectory == True
Using edm::ESGetToken
Removing unnecessary member variables

PR validation:

Workflow 135.9 does not abort anymore (but it won't succeed either, the real fix is different but independent of this PR). Limited matrix runs.

The bare "throw" will lead to abort because there is no exception to be thrown. On the other hand, the beamSpotHandle->position() will throw an exception if the product is missing (with much clearer message), so easiest fix is to just remove the check.

cmsbuild · 2020-06-02T18:42:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-06-02T18:47:37Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30082/15817

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaPhotonProducers/src/ConversionTrackProducer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935

cmsbuild · 2020-06-02T18:48:03Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

RecoEgamma/EgammaPhotonProducers

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

cms-bot commands are listed here

makortel · 2020-06-02T18:48:48Z

@cmsbuild, please test

cmsbuild · 2020-06-02T18:49:12Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6748/console Started: 2020/06/02 20:52

makortel · 2020-06-02T18:50:17Z

RecoEgamma/EgammaPhotonProducers/src/ConversionTrackProducer.cc

-  if (filterOnConvTrackHyp && !beamSpotHandle.isValid()) {
-    edm::LogError("Invalid Collection") << "invalid collection for the BeamSpot";
-    throw;
-  }


The beamspotHandle->position() on line 114/109 will throw an exception (with much clearer message) if the Handle is not valid.

(by the way, does

math::XYZVector beamSpot = math::XYZVector(beamSpotHandle->position());

really need to be repeated for each input track?)

cmsbuild · 2020-06-02T20:06:17Z

+1
Tested at: 3919d03
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0aedd8/6748/summary.html
CMSSW: CMSSW_11_2_X_2020-06-02-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-06-02T20:06:19Z

Comparison job queued.

cmsbuild · 2020-06-02T21:59:59Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2783666
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2783615
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 152 log files, 16 edm output root files, 36 DQM output files

perrotta · 2020-06-03T06:58:31Z

May I suggest then to leave the LogError, without throwing? Perhaps, while adding to the report also the beamSpotInputTag, so that one can have a complete picture from the log about who's the caller, and which is the missing beamSpot? (By the way: what is the extra info conveyed by beamspotHandle->position()?)
Moreover, as you correctly pointed out, the beam spot doesn't need to be re-defined for every track: we can profit of this PR to take it out of the for loop, then.

makortel · 2020-06-03T13:05:54Z

May I suggest then to leave the LogError, without throwing? Perhaps, while adding to the report also the beamSpotInputTag, so that one can have a complete picture from the log about who's the caller, and which is the missing beamSpot? (By the way: what is the extra info conveyed by beamspotHandle->position()?)

Why? If beamSpotHandle is not valid, beamSpotHandle->position() will throw the standard ProductNotFound exception that shows who tried to access which product.

perrotta · 2020-06-05T07:55:26Z

@makortel : Ok about removing the LogError as a whole from the ConversionTrackProducer
Please let me know if you intend to profit of this PR to also move out of the for loop the access to the beamSpot, as you already suggested in #30082 (comment)

cmsbuild · 2020-06-05T15:59:46Z

The code-checks are being triggered in jenkins.

makortel · 2020-06-05T16:02:17Z

@perrotta Done, I could not resists on doing a bit more modernization on the same go. I hope it is fine.

cmsbuild · 2020-06-05T16:05:52Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30082/15900

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEgamma/EgammaPhotonProducers/src/ConversionTrackProducer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935

cmsbuild · 2020-06-05T16:06:13Z

Pull request #30082 was updated. @perrotta, @cmsbuild, @slava77 can you please check and sign again.

makortel · 2020-06-05T16:11:25Z

@cmsbuild, please test

cmsbuild · 2020-06-05T16:11:44Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6844/console Started: 2020/06/05 20:54

perrotta · 2020-06-05T17:50:28Z

@perrotta Done, I could not resists on doing a bit more modernization on the same go. I hope it is fine.

Great! Thank you @makortel

cmsbuild · 2020-06-05T20:06:11Z

+1
Tested at: b1c35dd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0aedd8/6844/summary.html
CMSSW: CMSSW_11_2_X_2020-06-05-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-06-05T20:06:14Z

Comparison job queued.

cmsbuild · 2020-06-05T22:17:11Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2780497
DQMHistoTests: Total failures: 48
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2780399
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 152 log files, 16 edm output root files, 36 DQM output files

perrotta · 2020-06-06T07:40:13Z

+1

Technical
Jenkins tests pass

cmsbuild · 2020-06-06T07:40:39Z

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-06-06T12:13:33Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Jun 2, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jun 2, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 2, 2020

cmsbuild added tests-started and removed tests-pending labels Jun 2, 2020

makortel commented Jun 2, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jun 2, 2020

This was referenced Jun 2, 2020

Move EDProducers in HLT tracking validation sequences to Tasks #30087

Merged

Crash of many workflows in CMSSW_11_2_X_2020-06-01-2300 #30074

Closed

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

makortel changed the title ~~Remove redundant product validity check from ConversionTrackProducer~~ Remove redundant product validity check from ConversionTrackProducer and modernize it Jun 5, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 5, 2020

cmsbuild added tests-started and removed tests-pending labels Jun 5, 2020

cmsbuild added tests-approved and removed tests-started labels Jun 5, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jun 5, 2020

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

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

cmsbuild merged commit 4461193 into cms-sw:master Jun 6, 2020

makortel deleted the throwConversionTrackProducer branch June 6, 2020 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant product validity check from ConversionTrackProducer and modernize it #30082

Remove redundant product validity check from ConversionTrackProducer and modernize it #30082

makortel commented Jun 2, 2020 •

edited

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

makortel commented Jun 2, 2020

cmsbuild commented Jun 2, 2020 •

edited

makortel Jun 2, 2020

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

perrotta commented Jun 3, 2020

makortel commented Jun 3, 2020

perrotta commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

makortel commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

makortel commented Jun 5, 2020

cmsbuild commented Jun 5, 2020 •

edited

perrotta commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

perrotta commented Jun 6, 2020

cmsbuild commented Jun 6, 2020

silviodonato commented Jun 6, 2020

Remove redundant product validity check from ConversionTrackProducer and modernize it #30082

Remove redundant product validity check from ConversionTrackProducer and modernize it #30082

Conversation

makortel commented Jun 2, 2020 • edited

PR description:

PR validation:

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

makortel commented Jun 2, 2020

cmsbuild commented Jun 2, 2020 • edited

makortel Jun 2, 2020

Choose a reason for hiding this comment

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

cmsbuild commented Jun 2, 2020

perrotta commented Jun 3, 2020

makortel commented Jun 3, 2020

perrotta commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

makortel commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

makortel commented Jun 5, 2020

cmsbuild commented Jun 5, 2020 • edited

perrotta commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

cmsbuild commented Jun 5, 2020

perrotta commented Jun 6, 2020

cmsbuild commented Jun 6, 2020

silviodonato commented Jun 6, 2020

makortel commented Jun 2, 2020 •

edited

cmsbuild commented Jun 2, 2020 •

edited

cmsbuild commented Jun 5, 2020 •

edited