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

backport of CASTOR rechitcorrector and noise simulation fixes #23895

Merged
merged 3 commits into from Sep 11, 2018

Conversation

hvanhaev
Copy link
Contributor

This is a back port of changes already included in the master, as discussed in PR: #22424

This contains a fix in the CASTOR noise simulation, and updates in the rechitcorrector module.
It would be good to have these too in CMSSW_76X for analyses with 2015 data.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 20, 2018

A new Pull Request was created by @hvanhaev (Hans Van Haevermaet) for CMSSW_7_6_X.

It involves the following packages:

RecoLocalCalo/Castor
SimCalorimetry/CastorSim
SimGeneral/MixingModule

@perrotta, @cmsbuild, @civanch, @mdhildreth, @slava77 can you please review it and eventually sign? Thanks.
@makortel 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

@slava77
Copy link
Contributor

slava77 commented Jul 20, 2018

I don't think we even have a 7_6_X IBs set up.
@smuzaffar @fabiocos
please let me know if we do.
Thank you.

@smuzaffar
Copy link
Contributor

@slava77 , correct we do not have 76X IBs running.
@fabiocos if this is needed then should we enable 76X IBs for a week?

@slava77
Copy link
Contributor

slava77 commented Jul 31, 2018

@smuzaffar @fabiocos
can we revive a 76X IB?
I suppose that only one will be needed to run the tests for this PR (even if the tests are started or remade a few days after that IB is made)

@perrotta
Copy link
Contributor

backport of #22424

@perrotta
Copy link
Contributor

@hvanhaev : in case you want to proceed with this proposed backport, the same adjustments that guarantee the preservation of the current behaviour as in #23894 should be also ported here

@hvanhaev
Copy link
Contributor Author

hvanhaev commented Aug 28, 2018 via email

smuzaffar added a commit to cms-sw/cms-bot that referenced this pull request Aug 28, 2018
@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

@cmsbuild please test
(@smuzaffar : thank you for setting up the 76X IB!)

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 29, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30133/console Started: 2018/08/29 23:24

@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cms-sw cms-sw deleted a comment from cmsbuild Aug 29, 2018
@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 10, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30310/console Started: 2018/09/10 10:14

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 48 differences found in the comparisons
  • DQMHistoTests: Total files compared: 14
  • DQMHistoTests: Total histograms compared: 1009343
  • DQMHistoTests: Total failures: 530
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1008345
  • DQMHistoTests: Total skipped: 468
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -13 KiB( 13 files compared)
  • Checked 56 log files, 4 edm output root files, 14 DQM output files

@perrotta
Copy link
Contributor

There are differences in the comparisons.

They were already there also before the last revert, see #23895 (comment): my fault having overlooked them.

They must be explained before we can proceed with the backport. And one should also investigate why those difference show up only here in 76X (quite a lot of workflows) and not in 80X.

Please @hvanhaev have a look

@hvanhaev
Copy link
Contributor Author

hvanhaev commented Sep 10, 2018 via email

@perrotta
Copy link
Contributor

Hi Hans.

You can scroll down till the bottom of the page linked by "Comparison with the baseline”, which shows the results of the DQM comparisons, and go down to "validateJR". That page shows the comparisons for reco quantities (direct link: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_6_X_2018-09-09-0000+23895/28359/validateJR/)

One of the files in that page is "logRootQA.log": if you open it, towards its bottom you find the following list

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison
JR results differ 4 all_OldVSNew_RunMinBias2011Awf1000p0
JR results differ 6 all_OldVSNew_ZMM13TeVwf1330p0
JR results differ 4 all_OldVSNew_RunSinglePh2015Dwf134p911
JR results differ 7 all_OldVSNew_Higgs200ChargedTauswf9p0
JR results differ 6 all_OldVSNew_RunMinBias2011Awf1001p0
JR results differ 7 all_OldVSNew_TTbarFSwf5p1
JR results differ 2 all_OldVSNew_RunHI2011wf140p53
JR results differ 3 all_OldVSNew_TTbarwf25p0
JR results differ 2 all_OldVSNew_RunCosmics2011Awf4p22
JR results differ 1 all_OldVSNew_ZEEFS13wf135p4
JR results differ 6 all_OldVSNew_TTbarPUwf25202p0
SUMMARY Reco comparison results: 48 differences found in the comparisons

which tells you which workflows show differences.

You can access, any of them, e.g. "all_OldVSNew_ZMM13TeVwf1330p0":
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_6_X_2018-09-09-0000+23895/28359/validateJR/all_OldVSNew_ZMM13TeVwf1330p0/

@perrotta
Copy link
Contributor

@hvanhaev @fabiocos

Slava reminded me this old issue that was fixed since 8_0_X, but never backported to 7_6_X:
#13506

What we see here in the jenkins comparisons exactly corresponds to what was observed at the time, and has nothing to do with Castor in any case. That is therefore the origin of the (kind of random) differences observed, and it is confirmed that this pull request do not change the outputs with the default config which is implemented.

@perrotta
Copy link
Contributor

+1

  • Correct backport of the code needed to apply fixes for the analysis and reprocessings of Castor data, which do not change the previous behaviour in its default configuration: the last addition since previous reco signature just reinforced it.

@hvanhaev
Copy link
Contributor Author

hvanhaev commented Sep 10, 2018 via email

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@civanch you already signed, now the behaviour is fully backward compatible, in case please sign it again for reference

@cmsbuild cmsbuild merged commit fb9777b into cms-sw:CMSSW_7_6_X Sep 11, 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