RooFit::MultiProcess & TestStatistics part 5b: test RooGradMinimizerFcn#8694
Merged
guitargeek merged 2 commits intoAug 31, 2021
Conversation
|
Starting build on |
egpbos
force-pushed
the
RooFit_MultiProcess_PR_5b_fix_testRooGradMinimizerFcn
branch
from
July 16, 2021 19:07
5363f9d to
f102ea2
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Warnings:
And 218 more |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Warnings:
And 242 more |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Warnings:
And 218 more Failing tests: |
|
Build failed on windows10/cxx14. Errors:
And 10 more |
|
Build failed on mac11.0/cxx17. Warnings:
And 27 more |
|
Build failed on mac1014/python3. Warnings:
And 27 more |
Contributor
Author
|
Fixes #8697 |
egpbos
force-pushed
the
RooFit_MultiProcess_PR_5b_fix_testRooGradMinimizerFcn
branch
from
July 20, 2021 10:31
f102ea2 to
a8afdf2
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Warnings:
And 150 more |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Warnings:
And 174 more |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Warnings:
And 149 more Failing tests: |
|
Build failed on windows10/cxx14. Errors:
And 10 more |
|
Build failed on mac11.0/cxx17. Warnings:
|
|
Build failed on mac1014/python3. Warnings:
|
egpbos
force-pushed
the
RooFit_MultiProcess_PR_5b_fix_testRooGradMinimizerFcn
branch
from
July 20, 2021 13:41
a8afdf2 to
88096c3
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Warnings:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Warnings:
|
Contributor
Author
|
The last remaining Jenkins failures are not from this PR, so ready for review @lmoneta @guitargeek @hageboeck |
This fixes the RooGradMinimizer test in as minimal a way possible. Note that this still has a bunch of compile warnings.
Turns out the RooGradMinimizerFcn also still had a slight remaining bug, which was introduced by the "detemplatization" redesign of RooMinimizer in PR root-project#8596. The problem was that the RooMinimizer ctor, which calls the RooGradMinimizerFcn ctor, which calls synchronizeGradientParameterSettings... called _context->getMultiGenFcn() (_context is the same RooMinimizer that is creating the RooGradMinimizerFcn at this point...) and that in turn tries to do a dynamic_cast on _fcn, which is going to be the RooGradMinimizerFcn after it has been assigned after construction, but this is still in progress at that point, so the dynamic_cast fails, since _fcn is still nullptr. The fix was to remove a fortunately unused parameter in NumericalDerivator::SetInitialGradient, which removes the problematic _context->getMultiGenFcn() call. In addition, compilation warnings were produced by overloads that shadowed the 5-parameter DoDerivative/Gradient overloads. To silence the warnings, we just add five-parameter overloads that forward to the two-parameter ones. To implement full 5-parameter paths everywhere would require more work and discussion. Also some compilation errors on Windows fixed by using vectors instead of non-standard dynamic sized arrays.
egpbos
force-pushed
the
RooFit_MultiProcess_PR_5b_fix_testRooGradMinimizerFcn
branch
from
August 24, 2021 14:21
e34d451 to
261d9ea
Compare
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on mac11.0/cxx17. |
Contributor
Author
|
@phsft-bot build just on mac11.0/cxx17 I think the errored build above timed out before. I'm not sure what happened with the ROOT-debian10-i386/cxx14 build. The build itself was successful, or so it says at the end of the full log. |
|
Starting build on |
guitargeek
reviewed
Aug 31, 2021
Comment on lines
+246
to
+247
| virtual T DoDerivative(const T *x, unsigned int icoord, T * /*previous_grad*/, T * /*previous_g2*/, | ||
| T * /*previous_gstep*/) const |
Contributor
There was a problem hiding this comment.
Suggested change
| virtual T DoDerivative(const T *x, unsigned int icoord, T * /*previous_grad*/, T * /*previous_g2*/, | |
| T * /*previous_gstep*/) const | |
| virtual T DoDerivativeWithPreviousResult(const T *x, unsigned int icoord, T * /*previous_grad*/, T * /*previous_g2*/, | |
| T * /*previous_gstep*/) const |
What about giving these new functions a different name? There is no reason to also name it DoDerivative, and like this we avoid the compiler warning when only the old DoDerivative signature is overridden.
If a shorter but also descriptive name other than DoDerivativeWithPreviousResult comes to our mind, I would also be open to it.
guitargeek
reviewed
Aug 31, 2021
| T Derivative(const T *x, unsigned int icoord, T *previous_grad, T *previous_g2, | ||
| T *previous_gstep) const | ||
| { | ||
| return DoDerivative(x, icoord, previous_grad, previous_g2, previous_gstep); |
Contributor
There was a problem hiding this comment.
Suggested change
| return DoDerivative(x, icoord, previous_grad, previous_g2, previous_gstep); | |
| return DoDerivativeWithPrevResult(x, icoord, previous_grad, previous_g2, previous_gstep); |
guitargeek
reviewed
Aug 31, 2021
| /// In some cases, the gradient algorithm will use information from the previous step, these can be passed | ||
| /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size | ||
| /// so that these can be passed forward again as well at the call site, if necessary. | ||
| virtual void Gradient(const T *x, T *grad, T *previous_grad, T *previous_g2, T *previous_gstep) const |
Contributor
There was a problem hiding this comment.
Suggested change
| virtual void Gradient(const T *x, T *grad, T *previous_grad, T *previous_g2, T *previous_gstep) const | |
| virtual void GradientWithPrevResult(const T *x, T *grad, T *previous_grad, T *previous_g2, T *previous_gstep) const |
See equivalent comment about DoGradient.
guitargeek
reviewed
Aug 31, 2021
Comment on lines
+57
to
+58
| std::vector<double> Gradient(const std::vector<double> &v, double *previous_grad, double *previous_g2, | ||
| double *previous_gstep) const override |
Contributor
There was a problem hiding this comment.
Suggested change
| std::vector<double> Gradient(const std::vector<double> &v, double *previous_grad, double *previous_g2, | |
| double *previous_gstep) const override | |
| std::vector<double> GradientWithPrevResult(const std::vector<double> &v, double *previous_grad, double *previous_g2, | |
| double *previous_gstep) const override |
guitargeek
reviewed
Aug 31, 2021
Comment on lines
+44
to
+45
| virtual std::vector<double> Gradient(const std::vector<double> ¶meters, double */*previous_grad*/, double */*previous_g2*/, | ||
| double */*previous_gstep*/) const { return Gradient(parameters); }; |
Contributor
There was a problem hiding this comment.
Suggested change
| virtual std::vector<double> Gradient(const std::vector<double> ¶meters, double */*previous_grad*/, double */*previous_g2*/, | |
| double */*previous_gstep*/) const { return Gradient(parameters); }; | |
| virtual std::vector<double> GradientWithPrevResult(const std::vector<double> ¶meters, | |
| double * /*previous_grad*/, double * /*previous_g2*/, | |
| double * /*previous_gstep*/) const { return Gradient(parameters); }; |
Besides the name change, try to not have the character sequence */* but put a space after the first * to not confuse syntax highlighting of GitHub and many editors.
guitargeek
reviewed
Aug 31, 2021
| std::vector<double> previous_g2(functionGradient.G2().Data(), functionGradient.G2().Data() + functionGradient.G2().size()); | ||
| std::vector<double> previous_gstep(functionGradient.Gstep().Data(), functionGradient.Gstep().Data() + functionGradient.Gstep().size()); | ||
|
|
||
| std::vector<double> grad = fGradCalc.Gradient(par_vec, previous_grad.data(), previous_g2.data(), previous_gstep.data()); |
Contributor
There was a problem hiding this comment.
Suggested change
| std::vector<double> grad = fGradCalc.Gradient(par_vec, previous_grad.data(), previous_g2.data(), previous_gstep.data()); | |
| std::vector<double> grad = fGradCalc.GradientWithPrevResult(par_vec, previous_grad.data(), previous_g2.data(), previous_gstep.data()); |
guitargeek
reviewed
Aug 31, 2021
| // IMultiGradFunction overrides | ||
| double DoEval(const double *x) const override; | ||
| double DoDerivative(const double *x, unsigned int icoord) const override; | ||
| double DoDerivative(const double *x, unsigned int i_component, double *previous_grad, |
Contributor
There was a problem hiding this comment.
Suggested change
| double DoDerivative(const double *x, unsigned int i_component, double *previous_grad, | |
| double DoDerivativeWithPrevResult(const double *x, unsigned int i_component, double *previous_grad, |
guitargeek
reviewed
Aug 31, 2021
| return _grad[i_component].derivative; | ||
| } | ||
|
|
||
| double RooGradMinimizerFcn::DoDerivative(const double *x, unsigned int i_component, double *previous_grad, |
Contributor
There was a problem hiding this comment.
Suggested change
| double RooGradMinimizerFcn::DoDerivative(const double *x, unsigned int i_component, double *previous_grad, | |
| double RooGradMinimizerFcn::DoDerivativeWithPrevResult(const double *x, unsigned int i_component, double *previous_grad, |
guitargeek
reviewed
Aug 31, 2021
| std::vector<ROOT::Fit::ParameterSettings> ¶meter_settings) const | ||
| { | ||
| _gradf.SetInitialGradient(_context->getMultiGenFcn(), parameter_settings, _grad); | ||
| _gradf.SetInitialGradient(parameter_settings, _grad); |
Contributor
There was a problem hiding this comment.
Suggested change
| _gradf.SetInitialGradient(parameter_settings, _grad); | |
| _gradf.SetInitialGradient(nullptr, parameter_settings, _grad); |
There is also another solution to the problem of the dynamic cast in the constructor: you can just pass a nullptr as the first argument, which is unused anyway.
I would feel more comfortable with that, because it doesn't require changing the public interface of NumericalDerivator::SetInitialGradient.
guitargeek
reviewed
Aug 31, 2021
|
|
||
| // create gaussian parameters | ||
| double mean[n], sigma[n]; | ||
| std::vector<double> mean(n), sigma(n); |
Contributor
There was a problem hiding this comment.
Not a big deal, but all these std::vector that you introduced can also be std::array here.
guitargeek
requested changes
Aug 31, 2021
Contributor
There was a problem hiding this comment.
Hi Partick, thanks for the PR!
I have three requests, two of them I have already commented inline but I'll write a summary here:
-
I think it's better if the new overloads of
DoDerivativeandGradientthat take also the previous gradients have a different names likeDoDerivativeWithPrevResultandGradientWithPrevResultto avoid the member shadowing problem when only the originalDoDerivativeandGradientfunctions are overridden. Then you don't have to add all these trivial implementations of the new functions in the second commit, and we also don't create warnings for users that are implementing these IFunctions. -
We should avoid changing existing public interfaces if not necessary. In the second commit, you changed the interface of
NumericalDerivator::SetInitialGradient, but it's not absolutely necessary. In theRooGradMinimizerFcn, you can just pass anullptras the first parameter to avoid the problem that you described in the message of the second commit. -
The
testRooGradMinimizerFcnunit test takes way too long to finish. In the CI it took 200 seconds (release build) and on my machine with a debug build it took 5 minutes. I think before we can merge this, we have to do something about this, as unit tests should nor have a longer runtime than a few seconds. You have any idea how the test can be made faster without decreasing the coverage of the features that are to be tested?
Since this PR also touches the math component, we should also make sure that @lmoneta doesn't have any further change requests before merging.
lmoneta
approved these changes
Aug 31, 2021
lmoneta
left a comment
lmoneta
left a comment
Member
There was a problem hiding this comment.
The PR looks fine for me.
I agree with the comments of Jonas, but some of these changes I think can be applied later.
I will need to modify anyway some of these interface when adding support for the Hessian.
guitargeek
approved these changes
Aug 31, 2021
guitargeek
left a comment
guitargeek
left a comment
Contributor
There was a problem hiding this comment.
Ok, so then let's move on here and I'll address my comments myself in a followup PR.
guitargeek
added a commit
to guitargeek/root
that referenced
this pull request
Aug 31, 2021
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.
Contributor
|
I have opened a PR addressing my review comments, so we don't forget about this. |
guitargeek
added a commit
to guitargeek/root
that referenced
this pull request
Sep 1, 2021
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.
guitargeek
added a commit
that referenced
this pull request
Sep 1, 2021
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 #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.
Merged
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR reactivates and fixes the test for RooGradMinimizerFcn (introduced in #8596).
The first commit provides a way for external gradient calculators to use previous gradient information (gradient itself, second derivatives, step size) to calculate the next gradient values. Simultaneously, it allows the external calculator to pass back (via the same arrays to keep the redesign as minimal as possible, in anticipation of planned dedicated Hessian support) the second derivative and step sizes, so they can also be reused in the next gradient calculation. All of this reuse was already going on in Numerical2PGradientCalculator, but external gradient calculators had no access to this data, because the FCNGradAdaptor and IMultiGradFunction had no support for passing it back and forth. The commit also implements use of this mechanism in ExternalInternalGradientCalculator and in RooGradMinimizerFcn.
The second commit reactivates the (already existing) test, and fixes it, because it turned out it had a small remaining bug. The bug was fixed by removing an unused parameter from
NumericalGradient::SetInitialGradient, so that was two birds with one stone.