[RF] Follow up on "RooFit::MultiProcess & TestStatistics part 5b: test RooGradMinimizerFcn"#8940
Conversation
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
|
Build failed on windows10/cxx14. Errors:
|
|
I think I made up the BranchingPDF test myself as a sort of stress test, also for benchmarking purposes, but not based on realistic physics models. Perhaps good to check whether it makes sense at all (conceptually, as a representative of a real kind of model) to keep it. If not, maybe just ditch it. |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
Looks good, I require just to not move away from using std::vector in the test program
|
|
||
| // create gaussian parameters | ||
| std::vector<double> mean(n), sigma(n); | ||
| double mean[n], sigma[n]; |
There was a problem hiding this comment.
Why this change ?
n seems not to be a const int and it is not known at compile time. It will not work for some compilers (e.g. Windows)
There was a problem hiding this comment.
Oh, that was an accident! Originally, this PR started as a revert of Patricks last commit, but I see in the tests I reverted too much. Thanks for spotting this
This is a follow up on commits 1e452a8 and 1e452a8, almost reverting the second commit completely. It addresses the following comments by @guitargeek made in PR root-project#8694: 1. I think it's better if the new overloads of `DoDerivative` and `Gradient` that take also the previous gradients have a different names like `DoDerivativeWithPrevResult` and `GradientWithPrevResult` to avoid the member shadowing problem when only the original `DoDerivative` and `Gradient` functions are overridden. Then you don't have to add all these trivial implementations of the new functions in 1e452a8, and we also don't create warnings for users that are implementing these IFunctions. 2. We should avoid changing existing public interfaces if not necessary. In 1e452a8, you changed the interface of `NumericalDerivator::SetInitialGradient`, but it's not absolutely necessary. In the `RooGradMinimizerFcn`, you can just pass a `nullptr` as the first parameter to avoid the problem that you described in the message of commit 1e452a8.
The speedup is achieved by reducing the number of observables in the model, meaning less numeric integration is necessary. This was done because originally, the GradMinimizer.BranchingPDF test took between 3 and 5 minutes to run, depending on the setup.
8fa9b95 to
fd28c8f
Compare
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
4 participants
This is a follow up on #8694, implementing the requests made in the PR review.
More details in the commit messages.
Turning some observables into parameters in the GradMinimizer.BranchingPDF test should not reduce the coverage of the test, because for the minimizer the model actually gets more complex where there are more parameters.