[Math] Make building old C++ Minuit implementation optional #20032
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
Test Results 22 files 22 suites 3d 17h 11m 23s ⏱️ For more details on these failures, see this check. Results for commit 33a1506. ♻️ This comment has been updated with latest results. |
guitargeek
force-pushed
the
minuit_1
branch
5 times, most recently
from
7c5b819 to
bfd811a
Compare
guitargeek
force-pushed
the
minuit_1
branch
2 times, most recently
from
286e166 to
bd3826f
Compare
|
There seems to be unrelated commits in the PR (roofit value servers) |
|
They are related. Using Minuit 2 consistently unearthed some problems in RooFit, because Minuit 2 is more verbose with errors. So I use this PR to also see how I can fix these problems. The goal is to create spin-of PRs for the RooFit fixes (many are already in), and then finally come back to this PR and only include the Minuit changes. |
This avoids false positives for the parameters list in conditional fits.
Minuit 2 has been the default for 2 years, and one day we should consider not building ROOT with the old C++ TMinuit implementation anymore. To make it possible to try this out, introduce a new `minuit1` flag that is on by default. Building with `minuit1=OFF` will also help to ensure that we are actually using Minuit 2 in all out tests. With this, it was uncovered that the `stressRooFit` actually didn't use Minuit 2, even thought it's the default minimizer in RooFit as well. Therefore, the default minimizer setting in `stressRooFit`/`stressRooStats` are changed to `Minuit2`, reference files are updated, and new tests with the old Minuit 1 are added to keep previous test coverage.
There is no particular reason anymore to run this test with both Minuit 1 and Minuit 2, and it's a bit awkward that this is the only RooFit test that hardcodes the iteration over different minimizers.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Minuit 2 has been the default for 2 years, and one day we should consider not building ROOT with the old C++ TMinuit implementation anymore.
To make it possible to try this out, introduce a new
minuit1flag that is on by default.Building with
minuit1=OFFwill also help to ensure that we are actually using Minuit 2 in all out tests. With this, it was uncoveredthat the
stressRooFitactually didn't use Minuit 2, even thought it's the default minimizer in RooFit as well. Therefore, the default minimizer setting instressRooFit/stressRooStatsare changed toMinuit2, reference files are updated, and new tests with the old Minuit 1 are added to keep previous test coverage.