[Math] Make building old C++ Minuit implementation optional #20032
+182
−112
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 21 files 21 suites 3d 19h 32m 4s ⏱️ For more details on these failures, see this check. Results for commit 62b4847. ♻️ This comment has been updated with latest results. |
Since 6ff6266, Minuit 2 is always build with ROOT, so the fallback to the old Minuit in stressHistFactory has no effect.
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.
The RooFixedProdPdf used an antipattern that should be avoided, and it was actually causing some problems in fits with the new CPU evaluation backend. The problem was that the class held on to some RooAbsArgs for its evaluation in some containers that were separate from RooFit server-client interface (the container was the `std::unique_ptr<RooProdPdf::CacheElem>`). This commit fixes the situation by avoiding this cache element member completely and consistently using the server proxies instead.
After getting confused for some time why some fits don't work with the new CPU backend, I found one of the problems: the `std::ostringstream` was used incorrectly since 916a180 (commit from 10 years ago). The `std::ostringstream` constructor arguments do not initialize the stream with that string — the argument sets the open mode, not the contents. This can be seen with this little example in the interpreter. ```txt root [0] const char *pfx = "hello"; root [1] std::ostringstream os(pfx); root [2] os << " world"; root [3] os.str() (std::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::__string_type) " world" root [4] .q ``` The result is that the prefix doesn't get added, and hence it can happen that many nodes in the computation graph can have the same name, breaking the assumptions of the new RooFit evaluation backend.
This avoids false positives for the parameters list in conditional fits.
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.
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.