Remove some more getParameters() hacks
#1119
Conversation
WalkthroughRemoved two doc entries; removed three CachingSimNLL setters and internal flags; category parameters are always exposed and constraints applied when present; CascadeMinimizer now temporarily marks non-constant RooCategory parameters constant during RooMinimizer construction and removed dynamic hide/mask toggles. Changes
Sequence Diagram(s)sequenceDiagram
participant CM as CascadeMinimizer
participant NLL as CachingSimNLL
participant RM as RooMinimizer
CM->>NLL: getParameters()
CM->>CM: Mark non-constant RooCategory params constant (record changes)
CM->>RM: Construct RooMinimizer(NLL)
CM->>CM: Restore RooCategory params to original state
sequenceDiagram
participant NLL as CachingSimNLL
participant C as ConstraintSets
NLL->>C: Inspect constraint containers
alt constraints present
NLL->>C: Apply constraints during evaluate()
else
NLL->>NLL: Evaluate without applying constraints
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/CascadeMinimizer.cc (1)
🪛 GitHub Actions: clang-formatsrc/CascadeMinimizer.cc[error] clang-format formatting check failed. Run 'git-clang-format HEAD~' to fix code style issues in this file. ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/CascadeMinimizer.cc
Outdated
| minimizer_.reset(new RooMinimizer(nll_)); | ||
| if (simnll) simnll->setHideRooCategories(false); | ||
| RooArgSet params; | ||
| nll_.getParameters(nullptr, params); |
There was a problem hiding this comment.
warning: no matching member function for call to 'getParameters' [clang-diagnostic-error]
nll_.getParameters(nullptr, params);
^Additional context
/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:264: candidate function not viable: no known conversion from 'RooArgSet' to 'Bool_t' (aka 'bool') for 2nd argument
RooArgSet* getParameters(const RooAbsData* data, Bool_t stripDisconnected=kTRUE) const ;
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:273: candidate function not viable: no known conversion from 'RooArgSet' to 'Bool_t' (aka 'bool') for 2nd argument
virtual RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected=kTRUE) const ;
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:266: candidate function not viable: no known conversion from 'std::nullptr_t' to 'const RooAbsData' for 1st argument
RooArgSet* getParameters(const RooAbsData& data, Bool_t stripDisconnected=kTRUE) const {
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:270: candidate function not viable: no known conversion from 'std::nullptr_t' to 'const RooArgSet' for 1st argument
RooArgSet* getParameters(const RooArgSet& observables, Bool_t stripDisconnected=kTRUE) const {
^|
|
||
| // The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer. | ||
| for (RooAbsArg *arg : params) { | ||
| auto cat = dynamic_cast<RooCategory *>(arg); |
There was a problem hiding this comment.
warning: 'auto cat' can be declared as 'auto *cat' [llvm-qualified-auto]
| auto cat = dynamic_cast<RooCategory *>(arg); | |
| auto *cat = dynamic_cast<RooCategory *>(arg); |
| } | ||
|
|
||
| minimizer_.reset(); // avoid two copies in memory | ||
| minimizer_.reset(new RooMinimizer(nll_)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type 'pointer' (aka 'RooMinimizer *') with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
minimizer_.reset(new RooMinimizer(nll_));
^| } | ||
|
|
||
| minimizer_.reset(); // avoid two copies in memory | ||
| minimizer_.reset(new RooMinimizer(nll_)); |
There was a problem hiding this comment.
warning: use std::make_unique instead [modernize-make-unique]
src/CascadeMinimizer.cc:16:
+ #include <memory>| minimizer_.reset(new RooMinimizer(nll_)); | |
| minimizer_ = std::make_unique<RooMinimizer>(nll_); |
| minimizer_.reset(new RooMinimizer(nll_)); | ||
|
|
||
| // Change categories back to how they were | ||
| utils::setAllConstant(changedSet, false); |
There was a problem hiding this comment.
warning: argument comment missing for literal argument 'constant' [bugprone-argument-comment]
| utils::setAllConstant(changedSet, false); | |
| utils::setAllConstant(changedSet, /*constant=*/false); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
docs/part3/nonstandard.md(0 hunks)interface/CachingNLL.h(0 hunks)src/CachingNLL.cc(3 hunks)src/CascadeMinimizer.cc(1 hunks)
💤 Files with no reviewable changes (2)
- docs/part3/nonstandard.md
- interface/CachingNLL.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CascadeMinimizer.cc (2)
src/utils.cc (2)
setAllConstant(322-337)setAllConstant(322-322)interface/CascadeMinimizer.h (1)
CascadeMinimizer(14-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: review
- GitHub Check: Test with CMSSW_14_0_0_pre1 and ROOT v6.26/11
- GitHub Check: Test with CMSSW_14_1_0_pre4 and ROOT v6.30
- GitHub Check: Test with CMSSW_11_3_4 and ROOT v6.22/08
- GitHub Check: Test with CMSSW_14_2_0_pre2_ROOT632 and ROOT v6.32/06
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
🔇 Additional comments (3)
src/CachingNLL.cc (3)
1049-1086: LGTM: Always applying constraints when present is consistent with removing maskConstraintsThis aligns with the PR objective and simplifies behavior. Expect a performance change where prior runs relied on masked constraints during combinatorial scans. Please benchmark representative cards to confirm no unacceptable regressions.
Would you run a quick before/after timing on a few typical workspaces (with/without SIMNLL_GROUPCONSTRAINTS > 1 and MINIMIZER_multiMin_maskChannels=2) to quantify the impact?
1261-1274: Include RooCategory parameters in getParameters (ROOT ≥ 6.26 path): looks correctSymmetric change for the newer signature; consistent with the pre-6.26 branch.
1247-1259: CachingSimNLL::getParameters now correctly exposes RooCategory parametersRan a repository-wide search for any remaining uses of the old toggles/APIs (
setHideRooCategories(,setHideConstants(,setMaskConstraints(,MINIMIZER_multiMin_hideConstants,MINIMIZER_multiMin_maskConstraints) and found no occurrences. The addition ofcatParams_(andactiveCatParameters_) is therefore sufficient and safe.Looks good to merge.
src/CascadeMinimizer.cc
Outdated
| RooArgSet params; | ||
| nll_.getParameters(nullptr, params); | ||
| RooArgSet changedSet; | ||
|
|
||
| // The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer. | ||
| for (RooAbsArg *arg : params) { | ||
| auto cat = dynamic_cast<RooCategory *>(arg); | ||
| if (cat && !cat->isConstant()) { | ||
| cat->setConstant(); | ||
| changedSet.add(*cat); | ||
| } | ||
| } | ||
|
|
||
| minimizer_.reset(); // avoid two copies in memory | ||
| minimizer_.reset(new RooMinimizer(nll_)); | ||
|
|
||
| // Change categories back to how they were | ||
| utils::setAllConstant(changedSet, false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guarantee category restoration on exceptions when constructing RooMinimizer
If RooMinimizer construction throws, the categories you set constant remain constant, altering global state for subsequent fits. Wrap the construction in try/catch (or use RAII) to ensure categories are always restored.
Apply this diff:
// The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer.
for (RooAbsArg *arg : params) {
auto cat = dynamic_cast<RooCategory *>(arg);
if (cat && !cat->isConstant()) {
- cat->setConstant();
+ cat->setConstant(true);
changedSet.add(*cat);
}
}
- minimizer_.reset(); // avoid two copies in memory
- minimizer_.reset(new RooMinimizer(nll_));
+ try {
+ minimizer_.reset(); // avoid two copies in memory
+ minimizer_.reset(new RooMinimizer(nll_));
+ } catch (...) {
+ // Ensure we don't leave categories constant if construction fails
+ utils::setAllConstant(changedSet, false);
+ throw;
+ }
// Change categories back to how they were
utils::setAllConstant(changedSet, false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RooArgSet params; | |
| nll_.getParameters(nullptr, params); | |
| RooArgSet changedSet; | |
| // The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer. | |
| for (RooAbsArg *arg : params) { | |
| auto cat = dynamic_cast<RooCategory *>(arg); | |
| if (cat && !cat->isConstant()) { | |
| cat->setConstant(); | |
| changedSet.add(*cat); | |
| } | |
| } | |
| minimizer_.reset(); // avoid two copies in memory | |
| minimizer_.reset(new RooMinimizer(nll_)); | |
| // Change categories back to how they were | |
| utils::setAllConstant(changedSet, false); | |
| RooArgSet params; | |
| nll_.getParameters(nullptr, params); | |
| RooArgSet changedSet; | |
| // The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer. | |
| for (RooAbsArg *arg : params) { | |
| auto cat = dynamic_cast<RooCategory *>(arg); | |
| if (cat && !cat->isConstant()) { | |
| cat->setConstant(true); | |
| changedSet.add(*cat); | |
| } | |
| } | |
| try { | |
| minimizer_.reset(); // avoid two copies in memory | |
| minimizer_.reset(new RooMinimizer(nll_)); | |
| } catch (...) { | |
| // Ensure we don't leave categories constant if construction fails | |
| utils::setAllConstant(changedSet, false); | |
| throw; | |
| } | |
| // Change categories back to how they were | |
| utils::setAllConstant(changedSet, false); |
There was a problem hiding this comment.
Are you seriously suggesting catch (...) Mr Rabbit? 🙂
There was a problem hiding this comment.
@guitargeek Ha! Fair point! 😄 You're right to question that - catch (...) is pretty heavy-handed and not very idiomatic for this kind of code.
A few cleaner alternatives:
- RAII approach - Create a simple scope guard:
struct CategoryGuard {
RooArgSet& set;
CategoryGuard(RooArgSet& s) : set(s) {}
~CategoryGuard() { utils::setAllConstant(set, false); }
};-
Just assume RooMinimizer constructor is well-behaved - Given this is ROOT/RooFit code, the constructor probably doesn't throw in practice, so the current approach might be fine as-is.
-
Check if changedSet is empty before restoration - Add a simple guard:
if (!changedSet.empty()) {
utils::setAllConstant(changedSet, false);
}What's your preference? Or do you think the risk of RooMinimizer construction failing is low enough that we don't need the extra complexity at all?
guitargeek
force-pushed
the
not_masking_stuff
branch
from
ff11bd8 to
5c1b0e6
Compare
src/CascadeMinimizer.cc
Outdated
| minimizer_.reset(new RooMinimizer(nll_)); | ||
| if (simnll) simnll->setHideRooCategories(false); | ||
| RooArgSet params; | ||
| nll_.getParameters(static_cast<RooArgSet const*>(nullptr), params); |
There was a problem hiding this comment.
warning: no matching member function for call to 'getParameters' [clang-diagnostic-error]
nll_.getParameters(static_cast<RooArgSet const*>(nullptr), params);
^Additional context
/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:273: candidate function not viable: no known conversion from 'RooArgSet' to 'Bool_t' (aka 'bool') for 2nd argument
virtual RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected=kTRUE) const ;
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:264: candidate function not viable: no known conversion from 'const RooArgSet *' to 'const RooAbsData *' for 1st argument
RooArgSet* getParameters(const RooAbsData* data, Bool_t stripDisconnected=kTRUE) const ;
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:266: candidate function not viable: no known conversion from 'const RooArgSet *' to 'const RooAbsData' for 1st argument
RooArgSet* getParameters(const RooAbsData& data, Bool_t stripDisconnected=kTRUE) const {
^/github/home/micromamba/envs/clang-tidy-review/include/RooAbsArg.h:270: candidate function not viable: no known conversion from 'const RooArgSet *' to 'const RooArgSet' for 1st argument
RooArgSet* getParameters(const RooArgSet& observables, Bool_t stripDisconnected=kTRUE) const {
^
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/CascadeMinimizer.cc (2)
61-67: Prefer qualified pointer type and explicit boolean for clarity.Use a qualified pointer with auto and make the intent explicit when setting constants.
- for (RooAbsArg *arg : params) { - auto cat = dynamic_cast<RooCategory *>(arg); - if (cat && !cat->isConstant()) { - cat->setConstant(); - changedSet.add(*cat); - } - } + for (RooAbsArg *arg : params) { + auto *cat = dynamic_cast<RooCategory *>(arg); + if (cat && !cat->isConstant()) { + cat->setConstant(true); + changedSet.add(*cat); + } + }
69-73: Guarantee category restoration on exceptions and prefer make_unique.If RooMinimizer construction throws, categories remain constant. Wrap construction and always restore. Also modernize allocation.
- minimizer_.reset(); // avoid two copies in memory - minimizer_.reset(new RooMinimizer(nll_)); - - // Change categories back to how they were - utils::setAllConstant(changedSet, false); + try { + minimizer_.reset(); // avoid two copies in memory + minimizer_ = std::make_unique<RooMinimizer>(nll_); + } catch (...) { + // Ensure we don't leave categories constant if construction fails + utils::setAllConstant(changedSet, /*constant=*/false); + throw; + } + // Change categories back to how they were + utils::setAllConstant(changedSet, /*constant=*/false);Note: Ensure is included at the top of this translation unit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
docs/part3/nonstandard.md(0 hunks)interface/CachingNLL.h(0 hunks)src/CachingNLL.cc(3 hunks)src/CascadeMinimizer.cc(1 hunks)
💤 Files with no reviewable changes (2)
- docs/part3/nonstandard.md
- interface/CachingNLL.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CascadeMinimizer.cc (1)
src/utils.cc (2)
setAllConstant(322-337)setAllConstant(322-322)
🪛 GitHub Actions: clang-format
src/CascadeMinimizer.cc
[error] 1-1: git-clang-format formatting check failed. Command 'clang-format-18 --diff --binary clang-format-18 --commit $PR_BASE -- src/' exited with code 1. Run 'git-clang-format HEAD~' to fix code style issues.
src/CachingNLL.cc
[error] 1-1: git-clang-format formatting check failed. Command 'clang-format-18 --diff --binary clang-format-18 --commit $PR_BASE -- src/' exited with code 1. Run 'git-clang-format HEAD~' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test with CMSSW_14_1_0_pre4 and ROOT v6.30
- GitHub Check: Test with CMSSW_11_3_4 and ROOT v6.22/08
- GitHub Check: Test with CMSSW_14_2_0_pre2_ROOT632 and ROOT v6.32/06
- GitHub Check: Test with CMSSW_14_0_0_pre1 and ROOT v6.26/11
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: review
- GitHub Check: Compile (py3.12, root6.34.4)
🔇 Additional comments (5)
src/CascadeMinimizer.cc (1)
1-1: clang-format enforcement needed
CI is failing due to formatting violations. Please format all changed C/C++ files with our style and verify no diffs remain. For example:# List only the modified source files and run clang-format on them FILES=$(git diff --name-only $(git merge-base HEAD origin/main) | grep -E '\.(cc|hh|cpp|hpp|cxx|hxx)$') [ -n "$FILES" ] && clang-format -i --style=file $FILES # Confirm there are no remaining formatting diffs git diff --statEnsure you commit the formatting changes and re-run the CI checks.
src/CachingNLL.cc (4)
1049-1085: LGTM: Always apply constraints when present.The new gating correctly evaluates constraints whenever any constraint container is non-empty, consistent with removing maskConstraints. Behavior aligns with the PR objective.
1253-1257: LGTM: Category parameters are now always exposed (ROOT < 6.26).Including catParams_ in the returned set ensures categories are visible to clients like RooMinimizer without the old hide toggles.
1267-1271: LGTM: Category parameters are now always exposed (ROOT ≥ 6.26).Adding catParams_ to the output set in the new signature matches the intended behavior post-hack removal.
1-1: Please reformatsrc/CachingNLL.ccwith clang-format
The CI pipeline is currently failing its clang-format check on this file. To resolve, run:clang-format -i src/CachingNLL.ccThis will apply the project’s style rules. After formatting, commit the changes so that the CI clang-format job passes.
src/CascadeMinimizer.cc
Outdated
| RooArgSet params; | ||
| nll_.getParameters(static_cast<RooArgSet const*>(nullptr), params); | ||
| RooArgSet changedSet; |
There was a problem hiding this comment.
Fix RooFit getParameters overload usage (compilation failure across ROOT versions).
The current call matches neither the legacy pointer-returning API nor the newer ROOT ≥ 6.26 API. Use preprocessor guards to support both.
Apply this diff:
- RooArgSet params;
- nll_.getParameters(static_cast<RooArgSet const*>(nullptr), params);
- RooArgSet changedSet;
+ RooArgSet params;
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
+ // ROOT < 6.26: returns an owning pointer
+ std::unique_ptr<RooArgSet> tmp(nll_.getParameters(static_cast<const RooArgSet*>(nullptr)));
+ if (tmp) params.add(*tmp);
+#else
+ // ROOT >= 6.26: fills the provided output set
+ nll_.getParameters(static_cast<const RooArgSet*>(nullptr), params, /*stripDisconnected=*/true);
+#endif
+ RooArgSet changedSet;Add the missing include at the top of this file:
#include <memory>🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc around lines 56 to 58, the call to
nll_.getParameters(...) uses an overload that doesn't exist across all ROOT
versions and causes compilation failures; add #include <memory> at the top of
the file and replace the current getParameters invocation with version-guarded
calls: use the legacy pointer-returning overload when ROOT < 6.26 and the newer
overload that takes a RooArgSet reference when ROOT >= 6.26 (use preprocessor
checks such as checking ROOT_VERSION_CODE or a suitable ROOT macro), ensuring
you construct any temporary RooArgSet correctly and avoid passing nullptr for
the selector in the newer API.
Indeed, the RooMinimizer can't deal with floating category variables. But instead of hiding the categories when creating the RooMinimizer, one can also set them to constant and the minimizer will not complain too. This solution is less hacky than hooking into `getParameters()` of the CachingSimNLL. This was originally introduced by @gpetrucc in cms-analysis#574 Quoting from that PR: ``` For the dirty flag propagation to the NLL when the categories are changed, and option to avoid reporting the categories in getParameters() to avoid complaints from RooMinimizer ``` So indeed, hiding the categories was supposed to address these warnings, which are also gone when the categories are just set to constant.
Originally added in 2019 by @gpetrucc in cms-analysis#576. I quote that PR: ``` It is possible that already setting maskChannels=2 and SIMNLL_GROUPCONSTRAINTS=N make the gain from hideConstant and maskConstraints redundant in practice but I haven't tested since I developed them in the order they're listed here. ``` I think this is indeed the case. Maybe it would be good to check if removing these options indeed has relevant effect on performance, because reducing the number of these RooFit hacks helps to converge between Combine and upstream RooFit.
guitargeek
force-pushed
the
not_masking_stuff
branch
from
5c1b0e6 to
0225c81
Compare
| minimizer_.reset(); // avoid two copies in memory | ||
| minimizer_.reset(new RooMinimizer(nll_)); | ||
| if (simnll) simnll->setHideRooCategories(false); | ||
| std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)0)); |
There was a problem hiding this comment.
warning: no header providing "std::unique_ptr" is directly included [misc-include-cleaner]
src/CascadeMinimizer.cc:16:
+ #include <memory>| minimizer_.reset(); // avoid two copies in memory | ||
| minimizer_.reset(new RooMinimizer(nll_)); | ||
| if (simnll) simnll->setHideRooCategories(false); | ||
| std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)0)); |
There was a problem hiding this comment.
warning: use nullptr [modernize-use-nullptr]
| std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)0)); | |
| std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)nullptr)); |
|
Thanks a lot @guitargeek! I think we should test it with a relatively large model, with >>1 category, to understand if there is a drop in performance when these options are removed. In the example we run in CI, we have just 5 categories. We'll test it and get back to you. |
|
Thank you so much! |
2 participants
Remove some more
getParameters()hacks that were meant for performance optimizations, but are most likely redundant with other optimizations.Remove optional hiding of constants and constraints
Originally added in 2019 by @gpetrucc in #576.
I quote that PR:
I think this is indeed the case. Maybe it would be good to check if removing these options indeed has relevant effect on performance, because reducing the number of these RooFit hacks helps to converge between Combine and upstream RooFit.
Don't hide categories when creating the RooMinimizer
Indeed, the RooMinimizer can't deal with floating category variables.
But instead of hiding the categories when creating the RooMinimizer, one can also set them to constant and the minimizer will not complain too.
This solution is less hacky than hooking into
getParameters()of the CachingSimNLL.This was originally introduced by @gpetrucc in #574.
Quoting from that PR:
So indeed, hiding the categories was supposed to address these warnings, which are also gone when the categories are just set to constant.
Performace comparisons
I was running this example to see if the flags I suggest to remove actually benefited performance. But at least in that case, adding
--X-rtd MINIMIZER_multiMin_hideConstants --X-rtd MINIMIZER_multiMin_maskConstraintshad absolutely zero effect on the runtime.Summary by CodeRabbit