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

Final cleanup of new muon validation #22106

Merged
merged 3 commits into from Feb 7, 2018

Conversation

abbiendi
Copy link
Contributor

@abbiendi abbiendi commented Feb 4, 2018

This is the final cleanup for the new muon track validation.
It removes the code duplication and promotes the new code and configurations to default, while the old ones are deleted.

The next RelVals shall be compared with the special RelVals produced in CMSSW_10_0_0 to avoid discontinuity. For the same reason this PR should enter in time for the next round of RelVals production.

Obviously large differences are expected on the direct comparison of the new validation plots w.r.t. the old versions, due to the different setting of the parameters. This has been already tested and reported in:
https://indico.cern.ch/event/693825/contributions/2846510/attachments/1582881/2501491/NewMuonValidation.pdf

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22106/3238

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22106/3238/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22106/3238/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

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

It involves the following packages:

SimMuon/MCTruth
Validation/Configuration
Validation/RecoHI
Validation/RecoMuon

@perrotta, @civanch, @vazzolini, @kmaeshima, @mdhildreth, @dmitrijus, @cmsbuild, @jfernan2, @slava77, @vanbesien can you please review it and eventually sign? Thanks.
@swertz, @battibass, @kkrajczar, @rovere, @calderona, @HuguesBrun, @RylanC24, @jhgoh, @rociovilar, @trocino, @folguera, @drkovalskyi this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@civanch
Copy link
Contributor

civanch commented Feb 4, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25894/console Started: 2018/02/04 22:20

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2018

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22106/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2429425
  • DQMHistoTests: Total failures: 1193
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2428063
  • DQMHistoTests: Total skipped: 169
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.479999999981 KiB( 9 files compared)
  • Checked 110 log files, 9 edm output root files, 26 DQM output files

@civanch
Copy link
Contributor

civanch commented Feb 5, 2018

+1

@abbiendi
Copy link
Contributor Author

abbiendi commented Feb 5, 2018

In case anybody is willing to see results of the new validation vs the old one, I have been producing a lot of them in the last few days.

Comparisons using the special RelVals produced in CMSSW_10_0_0 vs the current (old) validation,

for FullSim:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_FullSim-2018-CMSSW_10_0_0_NewVal_vs_OldVal/

for FastSim and HIN:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_FastSim-HI-CMSSW_10_0_0_NewVal_vs_OldVal/

Given that 10_0_0 is a development release (and muon HLT has an obvious problem) one can see results for a stable release by going back to validation plots obtained in CMSSW_9_4_0, from private RelVals:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_CMSSW_9_4_0_NewVal_vs_OldVal/

There are also other comparisons obtained with the New muon validation, of 10_0_0 with 9_4_0, for FullSim:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_FullSim-2017-NewVal_CMSSW_10_0_0_vs_CMSSW_9_4_0/

and FastSim and HIN:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_FastSim-HI-NewVal_CMSSW_10_0_0_vs_CMSSW_9_4_0/

finally 2018 vs 2017 in 10_0_0:
https://cms-muonpog.web.cern.ch/cms-muonpog/test_NewValidation/plots_FullSim-NewVal-CMSSW_10_0_0_2018_vs_2017/

Obviously the results of this PR have been tested w.r.t. the customized validation and they give as expected the exactly same outputs.
Hence everything has been validated from the point of view of the validation code.

Since all this is a lot of work I hope it will converge soon, to avoid having to repeat it at yet another release. Also we need to avoid holes in the standard validation of the Muon POG, which is now guaranteed to proceed smoothly.
Thanks to the PPD (in particular to Francesco and Nadeesha) for having taken care of the many special RelVals that were necessary.

@perrotta
Copy link
Contributor

perrotta commented Feb 6, 2018

+1

  • This is mostly DQM, reco only marginally affected
  • Anyhow I verified that this PR replaces exactly the "default" muon validation stuffs with what is called with the prefix "New" in the current code. The "New" classes and configs which were in the release are therefore removed here, so that the muon validation when this PR will be merged will be the "New" one (already tested in the current pre-release). This is indeed the expected behavior for this PR

@fabiocos
Copy link
Contributor

fabiocos commented Feb 6, 2018

@dmitrijus @jfernan2 could you please check? This is the conclusion of a migration discussed at length, it would be good if we can get it in pre1

@parbol
Copy link
Contributor

parbol commented Feb 7, 2018

In the Muon POG we are strongly interested in getting this PR integrated in pre1. The motivation is simple: for pre1 we have requested special samples to do the validation that won't be produced again. If we miss pre1 we won't be able to validate the release afterwards. Giovanni has done an extensive validation and comparison (see links in this comment) and we are reasonably sure that no major issues are present. So in conclusion, we would really like to push for not missing pre1.

Please let me know in case you have any question or comments.

Thank you very much,

@jfernan2
Copy link
Contributor

jfernan2 commented Feb 7, 2018

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2018

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

@fabiocos
Copy link
Contributor

fabiocos commented Feb 7, 2018

+1

@cmsbuild cmsbuild merged commit 38e3ed6 into cms-sw:master Feb 7, 2018
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