-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Minuit2] Make function passing purely std::function
based
#20026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
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
519e1ff
to
eb9f00e
Compare
eb9f00e
to
93e9db5
Compare
hageboeck
approved these changes
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but note that an entry in the release notes might be in order:
FCNGradAdapter.h
is gone, so code might fail to compileFCNAdapter<xxx>
has to be converted toFCNAdapter
Test Results 22 files 22 suites 3d 20h 39m 24s ⏱️ Results for commit 22b91bb. ♻️ This comment has been updated with latest results. |
ab9c3d9
to
8a03ed8
Compare
pcanal
reviewed
Oct 3, 2025
pcanal
reviewed
Oct 3, 2025
pcanal
reviewed
Oct 3, 2025
ed33a82
to
5e3dae7
Compare
We should avoid leaking the ROOT Math classes into Minuit 2.
This commit proposes to use Minuit 2 directly via the `Minuit2Minimizer` instead of the `ROOT::Math::Minimizer` abstraction. The benefit is that we will be able to remove some interfaces in the `ROOT::Math` classes that were only motivated by the control that RooFit multiprocessing needs over Minuit 2, leaking implementation details.
The option to return external gradients in Minuit 2 internal space is only used for Minuit 2, and these flags should not be present in math function base classes. The same is true for the interface to calculate the gradient considering a previous result.
The base classes `FCNBase` and `FCNGradientBase` were already unified in 71806b3, and the Minuit code can be further simplified by merging also the derived adapter class for `std::function` objects. This will also make it easier for the users, because then don't have to use different classes depending on whether they pass external gradients or not. We also make the new `FCNAdapter` purely `std::function` based, instead of using a template parameter. This makes is possible to use Minuit2 directly from Python, as we can benefit from the `std::function` pythonization. It also makes the Minuit2 code more consistent, because `std::function` was already used for the G2 (second derivatives) and the Hessian.
5e3dae7
to
22b91bb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 makes is possible to use Minuit2 directly from Python, as we can benefit from the
std::function
pythonization.It also makes the Minuit2 code more consistent, because
std::function
was already used for the G2 (second derivatives) and the Hessian.Here is an example of how the base Minuit 2 can be used from Python now:
This makes the base Minuit 2 code easily usable from Python, as promised in the ROOT plan of work. With "base Minuit 2 code", I mean the same classes that would also be built with Minuit 2 standalone, not using any other ROOT abstraction layers like the
ROOT::Math::Minimizer
interface, theROOT::Math::Fitter
or RooFit.