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

set SERIAL on histogram fits to restore deterministic fits #23230

Merged
merged 1 commit into from May 21, 2018

Conversation

dan131riley
Copy link

In ROOT 6.12 with IMT enabled, the chi2 calculation for fits is chunked based on the number of cpus, changing the order of calculations. In some (possibly pathological) cases this can change the results, causing problems for the automated comparisons used for pull requests. This PR sets the SERIAL option on several fits to restore the deterministic ordering from prior to ROOT 6.12. Details are in issue #23105 and see also previous history in PR #22227

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @dan131riley (Dan Riley) for master.

It involves the following packages:

DQMServices/ClientConfig
RecoHI/HiTracking

@perrotta, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @slava77, @vanbesien can you please review it and eventually sign? Thanks.
@barvic, @makortel, @jazzitup, @echapon, @yenjie, @kurtejung, @mandrenguyen, @dgulhan, @yetkinyilmaz 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

@jfernan2
Copy link
Contributor

Hi @dan131riley Can I identify you as HI Validation DQM developer contact in
https://twiki.cern.ch/twiki/bin/viewauth/CMS/DQMContacts#Heavy_Ions
?
Thanks

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 17, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28012/console Started: 2018/05/17 17:13

@dan131riley
Copy link
Author

@jfernan2 this is strictly in my role in the core software team making the PR tests reproducible, I don't really know anything about the HI DQM

@cmsbuild
Copy link
Contributor

@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-23230/28012/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2740553
  • DQMHistoTests: Total failures: 2212
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2738158
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@dan131riley
Copy link
Author

The DQM comparisons are not what I expected, this will take some more research. Can the tests be re-run to see if they are consistent?

@davidlange6
Copy link
Contributor

davidlange6 commented May 18, 2018 via email

@cmsbuild
Copy link
Contributor

cmsbuild commented May 18, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28024/console Started: 2018/05/18 08:26

@slava77
Copy link
Contributor

slava77 commented May 18, 2018 via email

@cmsbuild
Copy link
Contributor

@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-23230/28024/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2740553
  • DQMHistoTests: Total failures: 2231
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2738139
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@slava77
Copy link
Contributor

slava77 commented May 19, 2018

+1

for #23230 d924c3f

@dan131riley
Copy link
Author

While the retest showed the same differences as the first test, even that doesn't tell us much as they used different baselines. I agree with @slava77 that this should be merged.

@fabiocos
Copy link
Contributor

@jfernan2 @dmitrijus could you please check and sign? I agree that we need to merge this PR if we really want to verify that it does what it is supposed to do, and I would like to have it in pre4 if possible, as these differences in tests are quite annoying

@jfernan2
Copy link
Contributor

+1

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

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 5401a4b into cms-sw:master May 21, 2018
@dan131riley dan131riley deleted the serial-fits branch June 13, 2018 01:40
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