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

Slightly refactor PF endcap cluster calibration #28960

Merged
merged 4 commits into from Mar 9, 2020
Merged

Slightly refactor PF endcap cluster calibration #28960

merged 4 commits into from Mar 9, 2020

Conversation

guitargeek
Copy link
Contributor

PR description:

In the PFEgammaAlgo, there is still this comment about code "stolen" from the PFECALSuperClusterAlgo:
https://github.com/cms-sw/cmssw/blob/master/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc#L1774

I was thinking of ways to avoid this duplication, but it's not quite trivial because the code diverged a bit. However, a bit part of the duplicate code serves to compute cluster energy corrections, so I could move this into the PFEnergyCalibration class which is used by both algos. I think this is a good step to avoid this code duplication, but I don't want to go further for now to not risk accidentally changing the logic. What do you think?

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.

@guitargeek guitargeek changed the title Refactor PF endcap cluster calibration Slightly PF endcap cluster calibration Feb 14, 2020
@guitargeek guitargeek changed the title Slightly PF endcap cluster calibration Slightly refactor PF endcap cluster calibration Feb 14, 2020
@cmsbuild cmsbuild added this to the CMSSW_11_1_X milestone Feb 14, 2020
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28960/13786

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoEcal/EgammaClusterAlgos
RecoParticleFlow/PFClusterTools
RecoParticleFlow/PFProducer
RecoParticleFlow/PFTracking

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

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Feb 14, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 14, 2020

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

@cmsbuild
Copy link
Contributor

+1
Tested at: 876e1dc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab1c14/4654/summary.html
CMSSW: CMSSW_11_1_X_2020-02-13-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2694005
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2693658
  • DQMHistoTests: Total skipped: 346
  • 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

@silviodonato
Copy link
Contributor

Do you have any comments? @perrotta @slava77

double ps2_energy_sum = 0.;
double ePS1 = 0.;
double ePS2 = 0.;
int condP1 = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if you make these two some "bool" variable, everything gets more intuitive

//getStatusCode() == 0 => active channel
// apply correction if all recHits are dead
if (channelStatus.getMap().find(strip1)->getStatusCode() == 0)
condP1 = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can break here

ESDetId strip2 = recH.recHitRef()->detId();
if (strip2 != ESDetId(0)) {
if (channelStatus.getMap().find(strip2)->getStatusCode() == 0)
condP2 = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can break here

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2020

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

@guitargeek
Copy link
Contributor Author

guitargeek commented Mar 6, 2020

Hi @perrotta, now it's ready to be tested again. I only kept the change with using the other overloaded version of the energyEm function in the ConvBremPFTrackFinder class, so that the interface of PFEnergyCalibration needs fewer public functions. This should not change the result.

@perrotta
Copy link
Contributor

perrotta commented Mar 6, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2020

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2020

+1
Tested at: 6b13a95
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab1c14/5039/summary.html
CMSSW: CMSSW_11_1_X_2020-03-05-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2680577
  • DQMHistoTests: Total failures: 40
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2680218
  • 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
Copy link
Contributor

perrotta commented Mar 9, 2020

+1

  • It allows some refactorization of the PF energy calibration
  • Jenkins tests pass and show no difference with the current reco outputs, as it should

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 9, 2020

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 48dccbc into cms-sw:master Mar 9, 2020
@guitargeek guitargeek deleted the PFEnergyCalibration_1 branch March 9, 2020 09:24
@slava77
Copy link
Contributor

slava77 commented Mar 11, 2020

[I'm not sure if this is the PR with the problem, but the error messages appear to point to this]

@smuzaffar @mrodozov
please remind me how to test on PPC interactively.
I suspect that the problem came from this PR, based on 140.52 working in CMSSW_11_1_X_2020-03-08-2300 but not in CMSSW_11_1_X_2020-03-09-2300.
In the latter we have

An exception of category 'StdException' occurred while
   [0] Processing  stream begin Run run: 152698 stream: 0
   [1] Calling method for module PFProducer/'particleFlowTmp'
   [2] Using EventSetup component PoolDBESSource/'GlobalTag' to make data PerformancePayload/'' in record PFCalibrationRcd
Exception Message:
A std::exception was thrown.
getrandom

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc7_ppc64le_gcc820/CMSSW_11_1_X_2020-03-08-2300/pyRelValMatrixLogs/run/140.52_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI/step2_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI.log
vs
https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc7_ppc64le_gcc820/CMSSW_11_1_X_2020-03-09-2300/pyRelValMatrixLogs/run/140.52_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI/step2_RunHI2010+RunHI2010+RECOHID10+RECOHIR10D11+HARVESTDHI.log

@slava77
Copy link
Contributor

slava77 commented Mar 12, 2020

ping

#28960 (comment)

@smuzaffar @mrodozov
please remind me how to test on PPC interactively.

@smuzaffar
Copy link
Contributor

@slava77 send me your ssh public key

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2020

I managed to get to the error in PerformancePayload.
It is apparently unrelated to this PR.
The same IB where the problem appeared apparently also has cms-sw/cmsdist#5573
the gdb stack trace points to a problem in boost and simply merging this PR on top of 08-2300 IB does not introduce any problems.

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

6 participants