Remove some more getParameters() hacks
#1119
Conversation
WalkthroughRemoved two doc entries and three CachingSimNLL setters/flags; category parameters and constraints are now always included/applied. CascadeMinimizer now temporarily marks non-constant RooCategory parameters constant during RooMinimizer construction and restores them, removing previous hide/mask toggles. Changes
Sequence Diagram(s)sequenceDiagram
participant CM as CascadeMinimizer
participant NLL as CachingSimNLL
participant RM as RooMinimizer
CM->>NLL: request parameter list / identify non-constant RooCategory params
note right of CM `#E8F6EF`: record original isConstant states
CM->>CM: set identified RooCategory params to constant
CM->>RM: construct RooMinimizer with NLL
RM-->>CM: RooMinimizer constructed
CM->>CM: restore RooCategory params to recorded states
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 extra constraints
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)src/CascadeMinimizer.cc (1)
⏰ 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). (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 {
^
src/CascadeMinimizer.cc
Outdated
|
|
||
| // 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); |
src/CascadeMinimizer.cc
Outdated
| } | ||
|
|
||
| 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_));
^
src/CascadeMinimizer.cc
Outdated
| } | ||
|
|
||
| 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_); |
src/CascadeMinimizer.cc
Outdated
| 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.
5c1b0e6 to
0225c81
Compare
src/CascadeMinimizer.cc
Outdated
| 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>
src/CascadeMinimizer.cc
Outdated
| 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! |
guitargeek
force-pushed
the
not_masking_stuff
branch
2 times, most recently
from
594b060 to
2b6facd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CascadeMinimizer.cc (1)
61-61: Minor style improvement: useauto *for pointer types.For clarity, consider declaring
catasauto *catto make the pointer type explicit.- auto cat = dynamic_cast<RooCategory *>(arg); + auto *cat = dynamic_cast<RooCategory *>(arg);Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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
🧠 Learnings (1)
📓 Common learnings
Learnt from: guitargeek
Repo: cms-analysis/HiggsAnalysis-CombinedLimit PR: 1163
File: test/CMakeLists.txt:270-277
Timestamp: 2025-11-05T10:16:10.017Z
Learning: In PR #1163, the file test/references/LHC-Higgs-coupling-models-text2workspace.out was present and correctly included in the repository, despite an earlier incorrect comment flagging it as missing.
🧬 Code graph analysis (1)
src/CascadeMinimizer.cc (1)
src/utils.cc (2)
setAllConstant(322-337)setAllConstant(322-322)
⏰ 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). (10)
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
🔇 Additional comments (3)
src/CachingNLL.cc (2)
1028-1028: LGTM! Simplified constraint application logic.Removing the
maskConstraints_conditional and always applying constraints when any constraint container is non-empty simplifies the logic and aligns with the PR's goal to eliminate dynamic masking toggles.
1220-1240: LGTM! Category parameters now always exposed.The removal of conditional
hideRooCategories_checks and unconditional inclusion of category parameters in both ROOT version paths correctly supports the new approach where categories are temporarily frozen during minimizer construction rather than hidden from parameter lists.src/CascadeMinimizer.cc (1)
516-518: LGTM! Simplified minimization logic.Removing the
hideConstantsandmaskConstraintsstatic flags simplifies themultipleMinimizefunction and aligns with the PR's goal to eliminate these RooFit workarounds. ThemaskChannelsflag remains for orthogonal channel-level masking functionality.
src/CascadeMinimizer.cc
Outdated
| std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)0)); | ||
| 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 : *nllParams) { | ||
| auto cat = dynamic_cast<RooCategory *>(arg); | ||
| if (cat && !cat->isConstant()) { | ||
| cat->setConstant(true); | ||
| 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.
Fix ROOT version compatibility for getParameters call.
The getParameters call on line 56 uses a signature that doesn't exist across all ROOT versions, causing compilation failures on ROOT ≥ 6.26. The API changed from returning an owning pointer to filling an output reference parameter.
Apply this diff to support both ROOT versions:
+#include <memory>
+
void CascadeMinimizer::remakeMinimizer() {
- std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)0));
RooArgSet changedSet;
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
+ std::unique_ptr<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)nullptr));
+#else
+ RooArgSet params;
+ nll_.getParameters((const RooArgSet*)nullptr, params, /*stripDisconnected=*/true);
+ std::unique_ptr<RooArgSet> nllParams(new RooArgSet(params));
+#endif
// The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer.
for (RooAbsArg *arg : *nllParams) {Note on exception safety: As discussed in past reviews, if RooMinimizer construction (line 69) throws an exception, the categories remain constant. While the risk is low given RooFit's typical behavior, consider whether RAII-based restoration would be more robust. However, fixing the compilation issue takes priority.
Based on learnings
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1119 +/- ##
==========================================
+ Coverage 20.69% 20.70% +0.01%
==========================================
Files 195 195
Lines 26155 26138 -17
Branches 3924 3920 -4
==========================================
+ Hits 5412 5413 +1
+ Misses 20743 20725 -18
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
guitargeek
force-pushed
the
not_masking_stuff
branch
from
2b6facd to
650ea0a
Compare
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
650ea0a to
dcab621
Compare
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
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.