RooFit::MultiProcess & TestStatistics part 3: Minuit2::NumericalDerivator #8567
Conversation
This class, based on original code by @lmoneta (https://github.com/lmoneta/root/blob/lvmini/math/mathcore/src/NumericalDerivator.cxx), replicates the Minuit2 numerical derivator calculations. Because it is a stand alone class, it can be used in RooFit to manually calculate gradients outside of Minuit2, e.g. in a parallelized way.
|
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 ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
And 19 more |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
And 17 more |
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
And 16 more |
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Starting build on |
egpbos
changed the title
|
Update: I have moved Originally, @lmoneta's NumericalDerivator was in mathcore. However, I think it fits well in Minuit2 as well, since the algorithms exactly try to mimic Minuit2's. Also, I have added some depedencies to Minuit2 parameter transformations that I think make it an even more logical fit in Minuit2 than to have to make mathcore depend on Minuit2. I guess in the end it's an arbitrary choice. |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
@phsft-bot build |
|
Starting build on |
There was a problem hiding this comment.
It looks to good to me. For clarity of the code, it would be nice to follow the ROOT coding convention for member function names and data member names, which are different than the RooFit ones.
It would be nice also to add a direct unit test of the class, but this could be eventually a separate PR
|
Build failed on ROOT-performance-centos8-multicore/default. Warnings:
|
Co-authored-by: Jonas Rembser <[email protected]> Co-authored-by: Stephan Hageboeck <[email protected]>
|
Starting build on |
From comments by @guitargeek and @hageboeck on PR root-project#8567 (root-project#8567): - Simplify copy constructor. - Inline simple getter/setters. - Replace fabs with std::abs.
|
Starting build on |
|
I have applied all suggestions, thanks again for the reviews @hageboeck and @guitargeek. If you agree with delaying the pointer -> reference refactoring (#8567 (comment)), mostly because it would be a lot easier and faster to check once #8596 and the PR after that are merged, then I think this is ready to merge. |
|
Build failed on ROOT-performance-centos8-multicore/default. Warnings:
|
From comments by @guitargeek and @hageboeck on PR root-project#8567 (root-project#8567): - Simplify copy constructor. - Inline simple getter/setters. - Replace fabs with std::abs. - Fix warning due to changed member ordering.
egpbos
force-pushed
the
RooFit_MultiProcess_PR_3_NumericalDerivator
branch
from
cfe1cad to
79170ea
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Warnings:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Failing tests: |
From comments by @guitargeek and @hageboeck on PR root-project#8567 (root-project#8567): - Simplify copy constructor. - Inline simple getter/setters. - Replace fabs with std::abs. - Fix warning due to changed member ordering.
egpbos
force-pushed
the
RooFit_MultiProcess_PR_3_NumericalDerivator
branch
from
79170ea to
948788e
Compare
|
Starting build on |
From comments by @guitargeek and @hageboeck on PR root-project#8567 (root-project#8567): - Simplify copy constructor. - Inline simple getter/setters. - Replace fabs with std::abs. - Fix warning due to changed member ordering.
egpbos
force-pushed
the
RooFit_MultiProcess_PR_3_NumericalDerivator
branch
from
948788e to
435fe8f
Compare
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Failing tests: |
There was a problem hiding this comment.
Thank you very much for the development of the Numericalderivator class for Minuit2 and addressing the comments by @lmoneta, @hageboeck, and me! The unaddressed comment with the references instead of pointers can be indeed addressed later. This PR is ready to be merged once the Jenkins tests pass.
|
The two failing Jenkins tests are not related to this PR, correct? |
guitargeek
dismissed
lmoneta’s stale review
The requested changes have been implemented by @egpbos
|
No, the Jenkins failures are fortunately not related to this PR. |
|
FYI @guitargeek @egpbos this seems to break the default configuration because |
From comments by @guitargeek and @hageboeck on PR root-project#8567 (root-project#8567): - Simplify copy constructor. - Inline simple getter/setters. - Replace fabs with std::abs. - Fix warning due to changed member ordering.
This PR introduces the NumericalDerivatorMinuit2 class, based on original code by @lmoneta (https://github.com/lmoneta/root/blob/lvmini/math/mathcore/src/NumericalDerivator.cxx). It replicates the Minuit2 numerical derivator calculations, but in a a stand alone class. This allows it to be used from RooFit to manually calculate gradients outside of Minuit2, e.g. in a parallelized way.
Checklist: