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

Do not put temporary TF1 into ROOT's global function pool #18636

Merged
merged 3 commits into from
May 12, 2017

Conversation

Dr15Jones
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

RecoVertex/BeamSpotProducer

@perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @tocheng, @VinInn, @rovere, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

Pull request #18636 was updated. @perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19689/console Started: 2017/05/09 18:23

TF1 gausy("localGausY","gaus");
TF1 gausz("localGausZ","gaus");
// also do not add to global list of functions
TF1 gausx("localGausX","gaus",false);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones The 3rd argument in the case of TF1 is a different type than in the case of a TFormula. For TF1, it is a TF1::EAddToList

   // Add to list behavior
   enum class EAddToList {
      kDefault,
      kAdd,
      kNo
   };

so this code actually 'still' request the addition to the list (i.e. kDefault)

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

Pull request #18636 was updated. @perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19696/console Started: 2017/05/09 20:01

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #18636 was updated. @perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 11, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19763/console Started: 2017/05/11 17:00

@Dr15Jones
Copy link
Contributor Author

I found one more case of a TF1 being added to the global list. That was the last case in the package.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Dr15Jones
Copy link
Contributor Author

This change is purely technical. I was forced to also add the extra arguments to the TF1 constructor which match the defaults used by ROOT:
https://root.cern.ch/doc/master/classTF1.html#a460b83312445ffa2877255767fe99b21

@Dr15Jones
Copy link
Contributor Author

type bug

@Dr15Jones
Copy link
Contributor Author

urgent

@perrotta
Copy link
Contributor

+1
I am not able to reproduce the crashes (same wfs as in #18620, slc6_amd64_gcc530, etc...)
Do not adding temporary TF1's to the root global list makes sense, however. All other defaults are maintained.
I trust @Dr15Jones for effectiveness of the bug-fix.

@Dr15Jones
Copy link
Contributor Author

To be more clear, I do not promise that this will fix the crash we are seeing. By getting this in, if we have another crash we will have eliminated one of the most obvious concentenders we presently have. Plus, not putting the TF1 into the global list is just a plain good idea to avoid other potential problems as well as make the code a bit more thread efficient.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 1714 differences found in the comparisons
  • DQMHistoTests: Total files compared: 24
  • DQMHistoTests: Total histograms compared: 1830138
  • DQMHistoTests: Total failures: 23215
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1806743
  • DQMHistoTests: Total skipped: 180
  • DQMHistoTests: Total Missing objects: 0
  • Checked 98 log files, 14 edm output root files, 24 DQM output files

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.

5 participants