RooFit::MultiProcess & TestStatistics part 4: RooMinimizerFcn refactoring#8569
Conversation
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
|
Build failed on mac11.0/cxx17. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
egpbos
force-pushed
the
RooFit_MultiProcess_PR_4_RooMinimizerFcn_refactoring
branch
from
470e35b to
c220919
Compare
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
| return res; | ||
|
|
||
| } | ||
| Int_t RooMinimizer::getPrintLevel() const |
There was a problem hiding this comment.
I would not outline these one-liners. They should better be inlined in the header file. There are also many such one-liners in RooAbsMininmizerFcn, but since they are copy pasted from the original file, I'm fine with keeping them there. But for new code, it would be nice to inline.
| return fvalue; | ||
| } | ||
|
|
||
| std::string RooMinimizerFcn::getFunctionName() const |
There was a problem hiding this comment.
These two functions getFunctionName and getFunctionTitle can also be inlined in the header to allow for better compiler optimization and improve code readability.
There was a problem hiding this comment.
Maybe, maybe not. I left some comments to bring down the includes in the headers. If you follow that route, it's actually better to forward declare RooAbsReal, and have those functions in the cxx.
If RooAbsReal needs to be included in the header, I would suggest inlining.
There was a problem hiding this comment.
Good point, if inlining means to add mode includes that would otherwise not be necessary it should not be done.
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
I have some suggestions regarding keeping headers small and with few dependencies to speed up compilation.
I also think that for an abstract class (an interface), RooAbsMinimizer has too many members and too many includes. Consider those suggestions optional, but if this code is touched, it could as well be modernised if time permits.
| class RooMinimizer; | ||
|
|
||
| virtual double DoEval(const double * x) const; | ||
| class RooMinimizerFcn : public RooAbsMinimizerFcn, public ROOT::Math::IBaseFunctionMultiDim { |
There was a problem hiding this comment.
Multiple inheritance from classes that are not abstract isn't nice in C++. Now all the stuff such as having two separate data sections, pointer changes on casting etc are going to happen.
If possible, make AbsMinimizerFcn really abstract, i.e. no members.
| #include <fstream> | ||
| #include <vector> | ||
|
|
||
| class RooMinimizer; | ||
| #include <RooAbsMinimizerFcn.h> | ||
|
|
||
| template<typename T> class TMatrixTSym; | ||
| using TMatrixDSym = TMatrixTSym<double>; | ||
|
|
||
| class RooMinimizerFcn : public ROOT::Math::IBaseFunctionMultiDim { | ||
|
|
||
| public: | ||
|
|
||
| RooMinimizerFcn(RooAbsReal *funct, RooMinimizer *context, | ||
| bool verbose = false); | ||
| RooMinimizerFcn(const RooMinimizerFcn& other); | ||
| virtual ~RooMinimizerFcn(); | ||
|
|
||
| virtual ROOT::Math::IBaseFunctionMultiDim* Clone() const; | ||
| virtual unsigned int NDim() const { return _nDim; } | ||
|
|
||
| RooArgList* GetFloatParamList() { return _floatParamList; } | ||
| RooArgList* GetConstParamList() { return _constParamList; } | ||
| RooArgList* GetInitFloatParamList() { return _initFloatParamList; } | ||
| RooArgList* GetInitConstParamList() { return _initConstParamList; } | ||
|
|
||
| void SetEvalErrorWall(Bool_t flag) { _doEvalErrorWall = flag ; } | ||
| /// Try to recover from invalid function values. When invalid function values are encountered, | ||
| /// a penalty term is returned to the minimiser to make it back off. This sets the strength of this penalty. | ||
| /// \note A strength of zero is equivalent to a constant penalty (= the gradient vanishes, ROOT < 6.24). | ||
| /// Positive values lead to a gradient pointing away from the undefined regions. Use ~10 to force the minimiser | ||
| /// away from invalid function values. | ||
| void SetRecoverFromNaNStrength(double strength) { _recoverFromNaNStrength = strength; } | ||
| void SetPrintEvalErrors(Int_t numEvalErrors) { _printEvalErrors = numEvalErrors ; } | ||
| Bool_t SetLogFile(const char* inLogfile); | ||
| std::ofstream* GetLogFile() { return _logfile; } | ||
| void SetVerbose(Bool_t flag=kTRUE) { _verbose = flag ; } | ||
|
|
||
| Double_t& GetMaxFCN() { return _maxFCN; } | ||
| Int_t GetNumInvalidNLL() const { return _numBadNLL; } | ||
|
|
||
| Bool_t Synchronize(std::vector<ROOT::Fit::ParameterSettings>& parameters, | ||
| Bool_t optConst, Bool_t verbose); | ||
| void BackProp(const ROOT::Fit::FitResult &results); | ||
| void ApplyCovarianceMatrix(TMatrixDSym& V); | ||
|
|
||
| Int_t evalCounter() const { return _evalCounter ; } | ||
| void zeroEvalCount() { _evalCounter = 0 ; } | ||
| /// Return a possible offset that's applied to the function to separate invalid function values from valid ones. | ||
| double getOffset() const { return _funcOffset; } | ||
|
|
||
| private: | ||
| void SetPdfParamErr(Int_t index, Double_t value); | ||
| void ClearPdfParamAsymErr(Int_t index); | ||
| void SetPdfParamErr(Int_t index, Double_t loVal, Double_t hiVal); | ||
|
|
||
| Bool_t SetPdfParamVal(int index, double value) const; | ||
| void printEvalErrors() const; | ||
| // forward declaration | ||
| class RooMinimizer; |
There was a problem hiding this comment.
Those forward declarations are on honourable attempt, but since the abstract version includes all those things, they are unnecessary.
To detect include errors in other classes, the best ordering is
#include "MyBaseClass.h"
#include "RooFitStuff.h"
#include "RooFitStuff2.h"
#include "RootStuff.h"
#include <stdlib>But again: Forward declare if possible.
| mutable double _maxFCN = -std::numeric_limits<double>::infinity(); | ||
| mutable double _funcOffset{0.}; | ||
| double _recoverFromNaNStrength{10.}; | ||
| mutable int _numBadNLL = 0; | ||
| mutable int _printEvalErrors = 10; | ||
| mutable int _evalCounter{0}; | ||
|
|
||
| unsigned int _nDim = 0; | ||
|
|
||
| Bool_t _optConst = kFALSE; | ||
|
|
||
| RooArgList *_floatParamList; | ||
| RooArgList *_constParamList; | ||
| RooArgList *_initFloatParamList; | ||
| RooArgList *_initConstParamList; | ||
|
|
||
| std::ofstream *_logfile = nullptr; | ||
| bool _doEvalErrorWall{true}; | ||
| bool _verbose; |
There was a problem hiding this comment.
Try not to have data members in abstract classes.
If there's no way around them, better make them private. Making them protected increases the amount of code that has access to them, and which will make refactoring harder, since there's more code that has to be touched.
| return fvalue; | ||
| } | ||
|
|
||
| std::string RooMinimizerFcn::getFunctionName() const |
There was a problem hiding this comment.
Maybe, maybe not. I left some comments to bring down the includes in the headers. If you follow that route, it's actually better to forward declare RooAbsReal, and have those functions in the cxx.
If RooAbsReal needs to be included in the header, I would suggest inlining.
|
@hageboeck Thanks for the extensive review! I will implement some changes based on that and @guitargeek's comments today. In the meantime, perhaps it would be useful to check out #8596 as well. That PR introduces the second concrete MinimizerFcn. Possibly, seeing |
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
This commit changes RooMinimizerFcn to become a derived class of RooAbsMinimizerFcn. This significantly reduces the size of the RooMinimizerFcn implementation, because most of it was logistics of synchronizing between RooFit and Minuit, which is now handled in RooAbsMinimizerFcn.
Some further additions, in preparation of upcoming PRs:
- The new optimizeConstantTerms function was factored out of Synchronize so it can be overridden by RooAbsMinimizerFcn classes that handle optimization differently.
- In a later PR, RooMinimizer will be refactored so that it no longer holds _func itself anymore, and hence cannot get to its name/title or optimization options. To handle this, RooMinimizerFcn now has three added functions for RooMinimizer's use:
- getFunctionName/Title
- setOptimizeConst
egpbos
force-pushed
the
RooFit_MultiProcess_PR_4_RooMinimizerFcn_refactoring
branch
from
ab4cee9 to
75dd184
Compare
|
Starting build on |
There was a problem hiding this comment.
Hi Patrick, I have two final requests: one to make the code build also on my system, and one related to the class version of RooMinimizer changing a ROOT class and forgetting to update the class version can have bad consequences for users, so it would really be better to do this already in this PR.
By the way, once the class version has been updated once in a given release development cycle, it doesn't need to be updated again in the same development cycle.
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
… review _func and _optConst had already been moved into RooAbsMinimizerFcn, since they are logically part of the function, not the minimizer. This commit removes them from RooMinimizer. The necessary functionality for using the MinimizerFcn's members from RooMinimizer had already been added, but mistakenly were left out of the previous commits. As a consequence of this move, setOffsetting also had to be added to the MinimizerFcn. For backwards compatibility, we leave in RooMinimizer::setOffsetting and pass on the call to the MinimizerFcn. RooMinimizer::optimizeConst also still had a duplicate implementation. Now it forwards to RooAbsMinimizerFcn::setOptimizeConst. Note that we also refactored RooAbsMinimizerFcn::setOptimizeConst and ::optimizeConstantTerms here. They were previously virtual but, in fact, we duplicated 95% of their implementations in all derived classes. Both are now implemented non-virtually, and in turn call a (much smaller) new virtual function RooAbsMinimizerFcn::setOptimizeConstOnFunction, which must be implemented in the derived classes to pass on the constant term optimization settings to the actual function to be minimized. Furthermore, some PR review issues are addressed in this commit: - Remove obsolete __ROOFIT_NOROOMINIMIZER preprocessor flags. - Rename snake_case methods into dromedaryCase. - Use unique_ptr for parameter lists. - Add doxygen for new functions. - Bumped RooMinimizer ClassDef version. - Added TClass include to fix compile issue on some systems.
egpbos
force-pushed
the
RooFit_MultiProcess_PR_4_RooMinimizerFcn_refactoring
branch
from
75dd184 to
61dbdae
Compare
|
Starting build on |
|
Ok, I made the two requested changes @guitargeek, thanks for the sharp reviews and support! Note that I force pushed an amended commit, so as not to clutter the commit history too much. |
There was a problem hiding this comment.
Looks good to me!
This PR splits up the RooMinimzerFunc into a RooAbsMinimizerFunc and an actual RooMinimizerFunc that inherits from it. This is to prepare for further RooFit developments (#8596). Most review comments have been addressed, for more details see the summary in #8569 (comment).
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Stop, stop!! e3b73a8 needs to be reverted before releasing ROOT!
That by the way also holds for any classes the are introduced in the MP-PRs. Please consider well wether they are allowed to support I/O or not. |
|
@guitargeek If the class had supported I/O before, the situation would be even worse: In general
No harm done here, since you can just revert the one line, but please double check that none of the other PRs destroy I/O capabilities of RooFit. |
|
Very sharp, thanks @hageboeck, I had no idea. Shall I do a revert PR for that one line? |
|
Ah great, you already did, thanks! |
|
Ah no, haha, it's an issue, ok, so shall I just make a PR to make the change or do you want to do it yourself? Is there some git procedure I should observe? I think |
|
I just created an issue from the comment, so yes, please add a PR! 👍 |
|
Ouch, that's painful! I totally fell for this, totally my fault. I suggested to increase the class number in this PR. Now I will always use this as an example for why magic numbers are dangerous :D Don't worry about this, @egpbos, I'll make a PR to revert the version number increase myself. I'll also add a comment to all RooFit classes with |
root-project#8569 set the ClassDef version to 1, while it was zero before. Zero was a special value meaning "no I/O needed" (see https://root.cern.ch/root/html534/guides/users-guide/AddingaClass.html#motivation). This commit resets it to zero. Fixes root-project#8652.
|
Ah, damn, I didn't read your comment in time @guitargeek, I already made a PR as well. Please feel free to close. And I agree, magic numbers bad :D |
|
Btw, if no I/O is necessary, is the ClassDef itself actually necessary? Alternatively (if it is necessary): what about a macro overload without version number argument? And a static check in ClassDef that fails compilation on zero, referring to the new versionless one instead. |
|
It's necessary if you want ROOT's type inspection capabilities to work, e.g. In most cases, this doesn't have advantages over |
#8569 set the ClassDef version to 1, while it was zero before. Zero was a special value meaning "no I/O needed" (see https://root.cern.ch/root/html534/guides/users-guide/AddingaClass.html#motivation). This commit resets it to zero. Fixes #8652.
|
Thanks Patrick for the PR! |
root-project#8569 set the ClassDef version to 1, while it was zero before. Zero was a special value meaning "no I/O needed" (see https://root.cern.ch/root/html534/guides/users-guide/AddingaClass.html#motivation). This commit resets it to zero. Fixes root-project#8652.
6 participants
This PR refactors RooMinimizerFcn into two separate classes: RooAbsMinimizerFcn and a new RooMinimizerFcn derived from that. RooAbsMinimizerFcn contains all common synchronization "logistics" necessary between RooFit and Minuit function parameters. The implementations of this abstract base class are then expected to supply the actual function evaluation parts. This allows us to later implement classes that calculate the gradient outside of Minuit. We will provide both a serial implementation of this and later on a parallel one using the MultiProcess framework.
Checklist: