[RF] Deprecate RooMinuit class (adapter between RooFit and minuit, got replaced by the more general RooMinimizer) by guitargeek · Pull Request #8609 · root-project/root

guitargeek · 2021-07-05T14:54:51Z

See commit descriptions.

This also includes an update of the reference file `stressRooFit_ref.root`, because the fit results differed at the 6th digit with the new RooMinimzer: ``` RooFitResult::isIdentical: constant parameter sigma_g2 differs in value: 4.18951 vs. 4.18946 (1.25945e-05) RooFitResult::isIdentical: final parameter frac differs in value: 0.656146 vs. 0.65613 (2.51561e-05) RooUnitTest ERROR: comparison of object RooFitResult::nll from result rf601_r2 fails comparison with counterpart in reference RooFitResult nll Test 38 : Interactive Minuit.....................................FAILED ```

In particular, this means means removing the `OldMinuit` value for the `Minimzer` command argument of `RooAbsPdf::fitTo()`. A more general minimizer adapter class called `RooMinimizer` was introduced in ROOT 5.24 (see commit 12389f7). With ROOT 6.08, the new RooMinimizer became the default interface to minuit in RooFit. Now, this commit removed the option to use the old `RooMinuit` in `RooAbsPdf::fitTo()` for 6.24.

The code of the RooMinuit class is moved to the RooFitLegacy subdirectories to clearly mark them as deprecated.

hageboeck

LGTM, but I have a few more suggestions.

hageboeck · 2021-07-05T15:09:28Z

roofit/histfactory/inc/RooStats/HistFactory/RooBarlowBeestonLL.h

  // void setAlwaysStartFromMin(Bool_t flag) { _startFromMin = flag ; }
  // Bool_t alwaysStartFromMin() const { return _startFromMin ; }

-  //RooMinuit* minuit() { return _minuit ; }
+  //RooMinimizer* minimizer() { return _minimizer ; }
  RooAbsReal& nll() { return const_cast<RooAbsReal&>(_nll.arg()) ; }
  // const RooArgSet& bestFitParams() const ;
  // const RooArgSet& bestFitObs() const ;


Maybe clean this up while you are at it ...

It's all code that will just rot because it's never being compiled.

hageboeck · 2021-07-05T15:09:59Z

roofit/histfactory/inc/RooStats/HistFactory/RooBarlowBeestonLL.h

  TIterator* _oiter ; //! Iterator of profile likelihood output parameter(s)
  */

-  // mutable RooMinuit* _minuit ; //! Internal minuit instance
+  // mutable RooMinimizer* _minimizer ; //! Internal minimizer instance

  // mutable Bool_t _absMinValid ; // flag if absmin is up-to-date
  // mutable Double_t _absMin ; // absolute minimum of -log(L)


... and here.

hageboeck · 2021-07-05T15:10:10Z

roofit/histfactory/src/HistoToWorkspaceFactory.cxx

    /*
    RooMsgService::instance().setGlobalKillBelow(RooFit::DEBUG) ;
    profile->getVal();
-    RooMinuit* minuit = ((RooProfileLL*) profile)->minuit();
+    RooMinimizer* minuit = ((RooProfileLL*) profile)->minimizer();
    minuit->setPrintLevel(5) ; // Print MINUIT messages
-    minuit->setVerbose(5) ; // Print RooMinuit messages with parameter 
+    minuit->setVerbose(5) ; // Print RooMinimizer messages with parameter 
                                // changes (corresponds to the Verbose() option of fitTo()
    */


... and here.

hageboeck · 2021-07-05T15:12:58Z

roofit/roofitcore/inc/RooProfileLL.h

  TIterator* _piter ; //! Iterator over profile likelihood parameters to be minimized 
  TIterator* _oiter ; //! Iterator of profile likelihood output parameter(s)


When you see (mutable) iterator members that are not serialised (//!), that is almost always a sign that a range-based for loop or a counting loop should be used. Less members, faster, easier to understand.

Just saying, maybe for another day ...

hageboeck · 2021-07-05T16:25:20Z

roofit/roofitcore/src/RooProfileLL.cxx

+  for(auto const& arg : obsSetOrig) {
+    auto var = dynamic_cast<RooRealVar*>(arg);
+    if(var == nullptr) break;


This pattern was used in a lot of old loops, and although it looks like a double check (both for checking that the loop isn't finished and that the type is correct), it's usually only checking if the loop is finished. In almost all cases, you can static_cast and run through the full collection. For good measure, I put an assert with a dynamic_cast into the loop. It brings extra debug info, and it doesn't cost anything in release builds.

Note also that if they were really checking the types, it wouldn't make any sense if you put {var, var, otherType, var} in a collection, because the last var would never be reached. That's most probably a bug.

Long story short: Those loops usually work with range-base and static_cast, and they will be much faster this way. Consider this as a standard way to modernise those kinds of loops (and make the var const whenever you can; here you can't). Also note that although auto const& looks like an awesome thing for "normal" collections, it's actually obfuscating the fact that you have a collection of pointers.

Suggested change

for(auto const& arg : obsSetOrig) {

auto var = dynamic_cast<RooRealVar*>(arg);

if(var == nullptr) break;

for(auto const arg : obsSetOrig) {

auto var = static_cast<RooRealVar*>(arg);

assert(dynamic_cast<RooRealVar*>(arg));

And if you want (I never went there), you can write an adapter that exposes a special kind of iterator:

for (auto const pvar : CastCollection<const RooRealVar*>(obsSetOrig) ) { pvar->GetName(); }

Maybe you can get some inspiration from #8421

Thanks for the comment! Very good point, I was too cautions here and didn't want to change the logic of the code. I'll do it now.

* use `std::unique_ptr` * use range-based loops * use overloads of `getObservables` and `getParameters that don't do heap allocation

guitargeek · 2021-07-06T10:17:27Z

@phsft-bot build

phsft-bot · 2021-07-06T10:17:33Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-06T10:20:01Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T10:20:00.007Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T10:58:19Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.hist.histdrawv7.test.gtest_hist_histdrawv7_test_histhistdrawv7testUnit

guitargeek · 2021-07-06T12:13:54Z

@phsft-bot build (ne more round of tests now that pcepsft10 is fixed)

phsft-bot · 2021-07-06T12:14:00Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-06T12:29:05Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-4.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T12:29:04.135Z] CMake Error at /mnt/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

guitargeek · 2021-07-06T13:40:20Z

Since the ROOT-ubuntu16 tests passed already in the previous round on a machine where it worked, we have now all the tested configurations passing.

phsft-bot · 2021-07-06T14:09:45Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T14:09:44.859Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T14:32:51Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T14:32:50.932Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T15:47:37Z

Build failed on mac1014/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T15:47:36.425Z] CMake Error at /Volumes/HD2/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T16:37:18Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-07-06T16:37:18.021Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

phsft-bot · 2021-07-06T16:47:51Z

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-06T16:47:50.592Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1045 (message):

guitargeek added 4 commits July 5, 2021 16:08

[RF] Move the RooMinuit class to RooFitLegacy

b1a3c74

The code of the RooMinuit class is moved to the RooFitLegacy subdirectories to clearly mark them as deprecated.

[RelNotes] Mention deprecation of RooMinuit class in release notes

d2ff910

guitargeek added the in:RooFit label Jul 5, 2021

guitargeek self-assigned this Jul 5, 2021

guitargeek requested review from Axel-Naumann, bellenot, couet, hageboeck and lmoneta as code owners July 5, 2021 14:54

guitargeek linked an issue Jul 5, 2021 that may be closed by this pull request

[RF] Remove "No RooMinimizer" code paths. #8604

Closed

hageboeck approved these changes Jul 5, 2021

View reviewed changes

[RF] Clean code in RooBarlowBeestonLL and HistoToWorkspaceFactory

defcdac

hageboeck reviewed Jul 5, 2021

View reviewed changes

[RF] Modernize RooProfileLL code

1bdeafd

* use `std::unique_ptr` * use range-based loops * use overloads of `getObservables` and `getParameters that don't do heap allocation

guitargeek force-pushed the __ROOFIT_NOROOMINIMIZER branch from e144515 to 1bdeafd Compare July 5, 2021 16:38

root-project deleted a comment from phsft-bot Jul 6, 2021

guitargeek merged commit 0088755 into root-project:master Jul 6, 2021

guitargeek deleted the __ROOFIT_NOROOMINIMIZER branch July 6, 2021 13:40

guitargeek mentioned this pull request Jul 6, 2021

RooFit::MultiProcess & TestStatistics part 4: RooMinimizerFcn refactoring #8569

Merged

2 tasks

		TIterator* _piter ; //! Iterator over profile likelihood parameters to be minimized
		TIterator* _oiter ; //! Iterator of profile likelihood output parameter(s)

Conversation

guitargeek commented Jul 5, 2021

Uh oh!

hageboeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Failing tests:

Uh oh!

guitargeek commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

guitargeek commented Jul 6, 2021

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

phsft-bot commented Jul 6, 2021

Errors:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants