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

Apply Naming Rules to BeamSpotObjects methods #36419

Merged
merged 1 commit into from Dec 13, 2021

Conversation

francescobrivio
Copy link
Contributor

@francescobrivio francescobrivio commented Dec 8, 2021

PR description:

Sister PR of #36348 to update the methods of BeamSpotObjects to follow CMSSW Naming Rules (specifically 2.8/2.9/2.10/2.11) so that:

  • setters methods start with set (and not with Set)
  • getters methods don't start anymore with the word Get

I used git cms-checkdeps -a to modify all the packages depending on the BeamSpotObjects class.

PR validation:

Code compiles

Backport:

Not a backport, no backport needed.

FYI @dzuolo @lguzzi

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36419/27236

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2021

A new Pull Request was created by @francescobrivio for master.

It involves the following packages:

  • Alignment/OfflineValidation (alca)
  • Calibration/TkAlCaRecoProducers (alca)
  • CondCore/BeamSpotPlugins (db)
  • CondCore/DBOutputService (db, alca)
  • CondCore/ESSources (db, alca)
  • CondFormats/BeamSpotObjects (db, alca)
  • CondTools/BeamSpot (db, alca)
  • DQM/BeamMonitor (dqm)
  • RecoVertex/BeamSpotProducer (reconstruction, alca)
  • SimTransport/PPSProtonTransport (simulation)

@malbouis, @civanch, @yuanchao, @pmandrik, @emanueleusai, @mdhildreth, @tvami, @cmsbuild, @ggovi, @jfernan2, @ahmad3213, @slava77, @jpata, @francescobrivio, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@adewit, @mtosi, @GiacomoSguazzoni, @JanFSchulte, @tocheng, @VinInn, @rovere, @tlampen, @mmusich, @threus, @dgulhan, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@francescobrivio
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0af2c2/21102/summary.html
COMMIT: c367a89
CMSSW: CMSSW_12_3_X_2021-12-07-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36419/21102/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 3 CMS deprecated warnings found, see summary page for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 9 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250608
  • DQMHistoTests: Total failures: 178
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3250408
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@francescobrivio
Copy link
Contributor Author

I see changes in EgammaV https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-07-2300+0af2c2/47345/10042.0_ZMM_13+2017+ZMM_13TeV_TuneCUETP8M1_GenSim+Digi+RecoFakeHLT+HARVESTFakeHLT+ALCA+Nano/EgammaV.html

--> is that coming from some other PR?

This PR is purely technical, just renaming methods.
So I guess differences must come from something else. Not sure how to check tho...

Also there are some es get() around: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0af2c2/21102/deprecated-warnings.log

I see they are in the test directory of CondCore/ESSources, should we make a separate PR to address those cases?

@tvami
Copy link
Contributor

tvami commented Dec 8, 2021

I see they are in the test directory of CondCore/ESSources, should we make a separate PR to address those cases?

Yes, I think that makes sense!

@francescobrivio
Copy link
Contributor Author

I see they are in the test directory of CondCore/ESSources, should we make a separate PR to address those cases?

Yes, I think that makes sense!

see #36429

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 9, 2021

I see changes in EgammaV https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2021-12-07-2300+0af2c2/47345/10042.0_ZMM_13+2017+ZMM_13TeV_TuneCUETP8M1_GenSim+Digi+RecoFakeHLT+HARVESTFakeHLT+ALCA+Nano/EgammaV.html

--> is that coming from some other PR?

I don't think so but I do not understand the change either, I have compared input and configs and everything looks equivalent... The most striking thing to me is that the 10042.0 wf is ZMM and the differences appear in the only 2 converted photons which appear in baseline and now are gone...

https://cmsweb.cern.ch/dqm/dev/start?runnr=1;dataset%3D/RelVal_wf10042_0_pr/CMSSW_12_3_X-PRcmssw_36419-21102/DQMIO;referenceshow%3Dall;referencenorm=False;referenceobj1%3Dother::/RelVal_wf10042_0_base/CMSSW_12_3_X-PRcmssw_36419-21102/DQMIO::;sampletype%3Doffline_relval;workspace%3DEverything;

In the past there have been random differences in some workflows related to eGamma but not at this level if I recall correctly....

The only thing I can suggest is to retrigger the tests again.... :-(

@francescobrivio
Copy link
Contributor Author

The only thing I can suggest is to retrigger the tests again.... :-(

Thanks @jfernan2! Ok let's try again the tests then!

@francescobrivio
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0af2c2/21178/summary.html
COMMIT: 6c4dc7f
CMSSW: CMSSW_12_3_X_2021-12-10-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36419/21178/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS deprecated warnings: 3 CMS deprecated warnings found, see summary page for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250704
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3250676
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

+1

@francescobrivio
Copy link
Contributor Author

Also after rebasing tests are clean (#36419 (comment)).
Deprecated warnings are being handled in a separate PR.

@tvami
Copy link
Contributor

tvami commented Dec 10, 2021

+1

@civanch
Copy link
Contributor

civanch commented Dec 11, 2021

+1

@francescobrivio
Copy link
Contributor Author

@cms-sw/reconstruction-l2 a kind ping to sign this PR when you are available 😄

@clacaputo
Copy link
Contributor

+reconstruction

@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 will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

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

7 participants