Improve logic for freezing and unfreezing disassiotiated parameters #1107
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
guitargeek
force-pushed
the
roomultipdf_2
branch
from
69b3549 to
628243d
Compare
guitargeek
force-pushed
the
roomultipdf_2
branch
from
628243d to
a3eb73e
Compare
guitargeek
force-pushed
the
roomultipdf_2
branch
from
a3eb73e to
0055132
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@coderabbitai Please review (testing this new feature) |
|
@clelange I'll start reviewing the PR changes for improving the parameter freezing/unfreezing logic. ✅ Actions performedReview triggered.
|
WalkthroughPublic API and utilities for freezing disassociated RooMultiPdf parameters were removed from headers and utils. src/CascadeMinimizer.cc now contains anonymous-namespace helpers that compute RooArgSets to freeze and applies freezing locally. Combine.cc no longer populates a global RooMultiPdf-parameter list. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Minimization routine
participant CM as CascadeMinimizer (src)
participant Helper as anon::freezeDiscParams()
participant Utils as utils::setAllConstant
Client->>CM: initiate minimize / improveOnce / hesse / minos
CM->>Helper: compute frozenSet (RooArgSet)
Helper-->>CM: frozenSet
CM->>Utils: setAllConstant(frozenSet, true) %% highlight: freeze
CM->>CM: perform minimization steps
CM->>Utils: setAllConstant(frozenSet, false) %% highlight: unfreeze
CM-->>Client: return results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential focus areas:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
♻️ Duplicate comments (13)
src/CascadeMinimizer.cc (13)
78-78: Use nullptr directly instead of static_cast.The static_cast to const RooArgSet* is unnecessary.
- std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; + std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(nullptr)};
99-99: Replace C-style cast with static_cast or nullptr.Use modern C++ casting or nullptr directly.
- std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters((const RooArgSet *)nullptr)); + std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters(nullptr));
279-279: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
318-318: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
359-359: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
407-407: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozen,true); + utils::setAllConstant(frozen, /*constant=*/true);
425-425: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozen,false); + utils::setAllConstant(frozen, /*constant=*/false);
434-434: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
464-464: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozen,true); + utils::setAllConstant(frozen, /*constant=*/true);
475-475: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozen,false); + utils::setAllConstant(frozen, /*constant=*/false);
528-528: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
546-546: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
691-691: Add argument comment for boolean literal.Add a comment for the boolean literal to improve readability.
- utils::setAllConstant(frozenSet,false); + utils::setAllConstant(frozenSet, /*constant=*/false);
🧹 Nitpick comments (1)
src/CascadeMinimizer.cc (1)
1-920: Ensure code formatting complianceThe CI flagged formatting issues but clang-format isn’t available in this environment. Please install clang-format (matching the project’s
.clang-format), then run:git-clang-format HEAD~ -- src/CascadeMinimizer.cc
Verify that no changes remain after formatting and commit the updated file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
interface/CascadeMinimizer.h(0 hunks)interface/utils.h(0 hunks)src/CascadeMinimizer.cc(21 hunks)src/Combine.cc(0 hunks)src/utils.cc(0 hunks)
💤 Files with no reviewable changes (4)
- interface/utils.h
- interface/CascadeMinimizer.h
- src/Combine.cc
- src/utils.cc
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CascadeMinimizer.cc (1)
src/utils.cc (3)
setAllConstant(321-336)setAllConstant(321-321)ret(59-63)
🪛 GitHub Actions: clang-format
src/CascadeMinimizer.cc
[error] 1-1: clang-format formatting changes detected in src/CascadeMinimizer.cc. Run 'git-clang-format HEAD~' to fix code style issues.
🔇 Additional comments (1)
src/CascadeMinimizer.cc (1)
797-804: Good refactoring of checkAlgoInType logic.The simplified logic with early returns improves readability.
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/CascadeMinimizer.cc (4)
68-69: Typo in function name: freezeDisassiotiatedParamsConsider renaming to freezeDisassociatedParams to avoid typos and ease future upstreaming/maintenance.
-RooArgSet freezeDisassiotiatedParams(RooArgSet const &multiPdfs, bool isVerbose = false) { +RooArgSet freezeDisassociatedParams(RooArgSet const &multiPdfs, bool isVerbose = false) {And update the single call site accordingly.
124-129: Static caching of runtime flags may ignore updates at runtimeUsing static bools will read values once per process. If the runtimedef flags can change during execution, this will not reflect updates.
If these flags are expected to change at runtime, drop the static or re-check on each call:
- static bool freezeDisassParams = runtimedef::get(std::string("MINIMIZER_freezeDisassociatedParams")); - static bool freezeDisassParams_verb = runtimedef::get(std::string("MINIMIZER_freezeDisassociatedParams_verbose")); + bool freezeDisassParams = runtimedef::get(std::string("MINIMIZER_freezeDisassociatedParams")); + bool freezeDisassParams_verb = runtimedef::get(std::string("MINIMIZER_freezeDisassociatedParams_verbose"));
387-394: Optionally include RooCategory in allRooMultiPdfParamsIf you intend to exclude all MultiPdf-associated parameters from nuisances, add categories too (harmless if nuisances contains only RooRealVar).
- for (RooAbsArg *a : *pdfPars) { - if(auto *v = dynamic_cast<RooRealVar *>(a)) { - if (!v->isConstant()) - allRooMultiPdfParams.add(*v); - } - } + for (RooAbsArg *a : *pdfPars) { + if (auto *v = dynamic_cast<RooRealVar *>(a)) { + if (!v->isConstant()) allRooMultiPdfParams.add(*v); + } else if (auto *cv = dynamic_cast<RooCategory *>(a)) { + if (!cv->isConstant()) allRooMultiPdfParams.add(*cv); + } + }
1-1: clang-format check failing in CIRun git-clang-format HEAD~ locally and commit the formatting changes to fix the pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CascadeMinimizer.cc(21 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CascadeMinimizer.cc (1)
src/utils.cc (3)
setAllConstant(321-336)setAllConstant(321-321)ret(59-63)
🪛 GitHub Actions: clang-format
src/CascadeMinimizer.cc
[error] 1-1: clang-format formatting check failed. Detected formatting differences in src/CascadeMinimizer.cc during the PR formatting check. Run 'git-clang-format HEAD~' to fix formatting.
⏰ 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). (9)
- GitHub Check: review
- GitHub Check: Test with CMSSW_11_3_4 and ROOT v6.22/08
- GitHub Check: Test with CMSSW_14_0_0_pre1 and ROOT v6.26/11
- GitHub Check: Test with CMSSW_14_2_0_pre2_ROOT632 and ROOT v6.32/06
- GitHub Check: Test with CMSSW_14_1_0_pre4 and ROOT v6.30
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.8, root6.22)
- GitHub Check: Compile (py3.10, root6.32.2)
| for (RooAbsArg *pdf : multiPdfs) { | ||
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; | ||
| for (RooAbsArg *a : *pdfPars) { | ||
| if(auto *v = dynamic_cast<RooRealVar *>(a)) { | ||
| if (!v->isConstant()) | ||
| multiPdfParams.add(*v); | ||
| } | ||
| bool ret = utils::freezeAllDisassociatedRooMultiPdfParameters((CascadeMinimizerGlobalConfigs::O().allRooMultiPdfs),(CascadeMinimizerGlobalConfigs::O().allRooMultiPdfParams),freeze); | ||
| return ret; | ||
| } else { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
RooCategory parameters never get added to multiPdfParams, so they won’t be frozen
You only add RooRealVar to multiPdfParams, but later attempt to freeze RooCategory too. As written, categories won’t be in the set and won’t be frozen. Add RooCategory during collection.
for (RooAbsArg *a : *pdfPars) {
- if(auto *v = dynamic_cast<RooRealVar *>(a)) {
- if (!v->isConstant())
- multiPdfParams.add(*v);
- }
+ if (auto *v = dynamic_cast<RooRealVar *>(a)) {
+ if (!v->isConstant()) multiPdfParams.add(*v);
+ } else if (auto *cv = dynamic_cast<RooCategory *>(a)) {
+ if (!cv->isConstant()) multiPdfParams.add(*cv);
+ }
}Also applies to: 109-119
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc around lines 77 to 85 (and similarly for lines 109 to
119), the loop only adds RooRealVar instances to multiPdfParams so RooCategory
parameters are never collected and thus not frozen; update the collection logic
to also detect RooCategory (use dynamic_cast<RooCategory *> or equivalent) and
add non-constant categories to multiPdfParams alongside RooRealVar, ensuring you
check isConstant() (or the correct const flag) before adding; replicate the same
change in the other occurrence at lines 109-119.
| auto *mpdf = dynamic_cast<RooMultiPdf *>(P); | ||
| RooAbsPdf *pdf = mpdf->getCurrentPdf(); | ||
| if (isVerbose) | ||
| std::cout << " Current active PDF - " << pdf->GetName() << '\n'; | ||
| std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters((const RooArgSet *)nullptr)); | ||
| RooStats::RemoveConstantParameters(&*pdfPars); // make sure still to ignore user set constants | ||
| multiPdfParams.remove(*pdfPars); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard dynamic_cast result and modernize casts/usage
Avoid potential null deref if P isn’t a RooMultiPdf, and prefer static_cast/nullptr and smart-pointer accessors.
- auto *mpdf = dynamic_cast<RooMultiPdf *>(P);
- RooAbsPdf *pdf = mpdf->getCurrentPdf();
+ auto *mpdf = dynamic_cast<RooMultiPdf *>(P);
+ if (!mpdf) continue;
+ RooAbsPdf *pdf = mpdf->getCurrentPdf();
if (isVerbose)
std::cout << " Current active PDF - " << pdf->GetName() << '\n';
- std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters((const RooArgSet *)nullptr));
- RooStats::RemoveConstantParameters(&*pdfPars); // make sure still to ignore user set constants
+ std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))};
+ RooStats::RemoveConstantParameters(pdfPars.get()); // make sure still to ignore user set constants
multiPdfParams.remove(*pdfPars);📝 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.
| auto *mpdf = dynamic_cast<RooMultiPdf *>(P); | |
| RooAbsPdf *pdf = mpdf->getCurrentPdf(); | |
| if (isVerbose) | |
| std::cout << " Current active PDF - " << pdf->GetName() << '\n'; | |
| std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters((const RooArgSet *)nullptr)); | |
| RooStats::RemoveConstantParameters(&*pdfPars); // make sure still to ignore user set constants | |
| multiPdfParams.remove(*pdfPars); | |
| auto *mpdf = dynamic_cast<RooMultiPdf *>(P); | |
| if (!mpdf) continue; | |
| RooAbsPdf *pdf = mpdf->getCurrentPdf(); | |
| if (isVerbose) | |
| std::cout << " Current active PDF - " << pdf->GetName() << '\n'; | |
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; | |
| RooStats::RemoveConstantParameters(pdfPars.get()); // make sure still to ignore user set constants | |
| multiPdfParams.remove(*pdfPars); |
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc around lines 95 to 101, guard the dynamic_cast and
modernize pointer usage: check that dynamic_cast<RooMultiPdf*>(P) succeeded
before using mpdf and also ensure mpdf->getCurrentPdf() isn't null; replace
C-style cast (const RooArgSet *)nullptr with nullptr, construct a
std::unique_ptr<RooArgSet> pdfPars using pdf->getParameters(nullptr) and pass
pdfPars.get() to RooStats::RemoveConstantParameters and dereference
pdfPars.get() when calling multiPdfParams.remove(...); if any cast or pdf
retrieval fails, handle it (return/continue or log) to avoid null deref.
| } | ||
|
|
||
| return (iret != 1) ? true : false; | ||
| return iret != 1; |
There was a problem hiding this comment.
Success criteria for Minos look inverted; treat 0 and 1 as success
Elsewhere (minimize path) status 0 or 1 are accepted as success. return iret != 1 incorrectly returns true for other non-1 error codes.
- return iret != 1;
+ return iret == 0 || iret == 1;📝 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.
| return iret != 1; | |
| return iret == 0 || iret == 1; |
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc around line 331, the function currently treats only
iret == 1 as success; update the return so that both 0 and 1 are treated as
success and the function returns failure only when iret is neither 0 nor 1
(i.e., return true for failure and false when iret equals 0 or 1).
| } | ||
|
|
||
| return (iret != 1) ? true : false; | ||
| return iret != 1; |
There was a problem hiding this comment.
**Unify Hesse success criteria with minimize()
As above, return iret != 1 will treat other errors as success. Align with 0/1-success convention.
- return iret != 1;
+ return iret == 0 || iret == 1;📝 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.
| return iret != 1; | |
| - return iret != 1; | |
| + return iret == 0 || iret == 1; |
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc at line 365, the function currently returns "iret !=
1" which treats non-1 error codes as success; change the return to match the
minimize() success convention by returning "iret == 0" so only a 0 code is
treated as success (replace the existing expression with the same success check
used in minimize()).
|
Thanks for having taken a look and fixed up this PR! Do you intend to merge this soon? The next step for me here would be to conditionally remove the code completely, if built on the upcoming ROOT >= 6.38. All the logic to freeze disconnected parameters was not moved to RooFit itself, where this is now taken care of automatically whenever the RooMinimizer is engaged: |
|
@guitargeek something is wrong here, the scan before this PR Can be reproduced with: Also getting |
|
Thanks! I'll take a look. |
|
@anigamova, I understand now what is going on. The caching NLL classes override I'm working in some other PRs on avoiding some hacks on the combine side, to make the reported parameters more consistent between the RooFit pdfs and the Combine NLL. |
This functionality is intended to be upstreamed to RooFit, and therefore refactored in one portable function that can be moved to the RooFit helpers. The major change is that the function that freezes the parameters now returns a set with all parameters where the constant change was toggled. This allows us to easily un-freeze these parameters later, without having to carry around in the global state the full list of original floating parameters
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
guitargeek
force-pushed
the
roomultipdf_2
branch
from
7679114 to
a4fb8ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/CascadeMinimizer.cc (1)
99-99: Unify nullptr casting style.Line 78 uses
static_cast<const RooArgSet *>(nullptr)while this line uses a C-style cast. Prefer the modern static_cast for consistency.- std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters((const RooArgSet *)nullptr)); + std::unique_ptr<RooArgSet> pdfPars(pdf->getParameters(static_cast<const RooArgSet *>(nullptr)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
interface/CascadeMinimizer.h(0 hunks)interface/utils.h(0 hunks)src/CascadeMinimizer.cc(21 hunks)src/Combine.cc(0 hunks)src/utils.cc(0 hunks)
💤 Files with no reviewable changes (4)
- src/utils.cc
- src/Combine.cc
- interface/CascadeMinimizer.h
- interface/utils.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/CascadeMinimizer.cc (2)
src/ProfilingTools.cc (6)
get(41-44)get(41-41)get(57-64)get(57-57)get(65-67)get(65-65)src/utils.cc (3)
setAllConstant(321-336)setAllConstant(321-321)ret(59-63)
🔇 Additional comments (8)
src/CascadeMinimizer.cc (8)
232-232: LGTM: Freeze/unfreeze pattern correctly applied.The new pattern obtains a frozenSet at the start and restores it before returning, which correctly tracks only the parameters whose constant state was toggled.
Also applies to: 297-297
313-315: LGTM: Minos freeze/unfreeze logic updated correctly.The function properly freezes parameters before running Minos and unfreezes them afterward using the new frozenSet pattern.
Also applies to: 331-336
375-377: LGTM: Hesse freeze/unfreeze logic updated correctly.The function follows the new frozenSet pattern consistently.
415-452: LGTM: Multiple freeze/unfreeze operations coordinated correctly.The function correctly manages two levels of freezing: the frozenSet from freezeDiscParams and the frozen set for POIs and nuisances. Both are properly unfrozen in the correct order.
468-564: LGTM: Minimize function properly manages freezing across multiple code paths.The function correctly applies the frozenSet pattern at the entry (line 468) and ensures cleanup at exit (line 564), with additional freezing/unfreezing in specific code paths (lines 482-493, 543-546) handled appropriately.
696-709: LGTM: Freeze/unfreeze correctly scoped within the combination loop.Each iteration properly freezes disassociated parameters before fitting and unfreezes them afterward.
815-822: LGTM: Simplified with early return pattern.The refactored logic is clearer and more concise.
78-78: Known regression being investigated: getParameters() calls on RooMultiPdfs.According to the PR objectives, this PR introduces a regression where scans differ and an error "FCN function has zero parameters" occurs. The PR author (guitargeek) identified that calling
getParameters()directly on RooMultiPdfs (as done on these lines) produces inconsistent results because caching NLL classes overridegetParameters()to return parameters as Combine expects.The author is working on other PRs to make reported parameters consistent between RooFit PDFs and Combine NLL, which should resolve this issue. This is noted here for awareness, but the fix is expected in a follow-up PR.
Also applies to: 99-99
⛔ Skipped due to 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.
| CascadeMinimizerGlobalConfigs::O().allRooMultiPdfParams.Print(); | ||
| namespace { | ||
|
|
||
| RooArgSet freezeDisassiotiatedParams(RooArgSet const &multiPdfs, bool isVerbose = false) { |
There was a problem hiding this comment.
Fix typo in function name.
The function is named freezeDisassiotiatedParams but should be freezeDisassociatedParams (missing 'c' in "disassociated").
Apply this diff to fix the typo:
-RooArgSet freezeDisassiotiatedParams(RooArgSet const &multiPdfs, bool isVerbose = false) {
+RooArgSet freezeDisassociatedParams(RooArgSet const &multiPdfs, bool isVerbose = false) {And update the call site:
- return freezeDisassiotiatedParams(multiPdfs, freezeDisassParams_verb);
+ return freezeDisassociatedParams(multiPdfs, freezeDisassParams_verb);Also applies to: 133-133
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc at lines ~68 and ~133, the function name has a typo:
`freezeDisassiotiatedParams` should be `freezeDisassociatedParams`; rename the
function definition and any declarations to `freezeDisassociatedParams` (fix the
missing 'c'), and update all call sites accordingly (including the one around
line 133 and any forward declarations or header prototypes) so identifiers match
exactly.
| RooArgSet allRooMultiPdfParams; | ||
| for (RooAbsArg *pdf : CascadeMinimizerGlobalConfigs::O().allRooMultiPdfs) { | ||
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; | ||
| for (RooAbsArg *a : *pdfPars) { | ||
| if(auto *v = dynamic_cast<RooRealVar *>(a)) { | ||
| if (!v->isConstant()) | ||
| allRooMultiPdfParams.add(*v); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
RooCategory parameters not collected.
Similar to the issue flagged in past review comments for lines 77-85, this loop only collects RooRealVar parameters but not RooCategory parameters. If any RooMultiPdf has category parameters that should be considered, they will be missed.
Apply this diff to also collect RooCategory:
for (RooAbsArg *pdf : CascadeMinimizerGlobalConfigs::O().allRooMultiPdfs) {
std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))};
for (RooAbsArg *a : *pdfPars) {
if(auto *v = dynamic_cast<RooRealVar *>(a)) {
if (!v->isConstant())
allRooMultiPdfParams.add(*v);
+ } else if(auto *cv = dynamic_cast<RooCategory *>(a)) {
+ if (!cv->isConstant())
+ allRooMultiPdfParams.add(*cv);
}
}
}📝 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 allRooMultiPdfParams; | |
| for (RooAbsArg *pdf : CascadeMinimizerGlobalConfigs::O().allRooMultiPdfs) { | |
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; | |
| for (RooAbsArg *a : *pdfPars) { | |
| if(auto *v = dynamic_cast<RooRealVar *>(a)) { | |
| if (!v->isConstant()) | |
| allRooMultiPdfParams.add(*v); | |
| } | |
| } | |
| } | |
| RooArgSet allRooMultiPdfParams; | |
| for (RooAbsArg *pdf : CascadeMinimizerGlobalConfigs::O().allRooMultiPdfs) { | |
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters(static_cast<const RooArgSet *>(nullptr))}; | |
| for (RooAbsArg *a : *pdfPars) { | |
| if(auto *v = dynamic_cast<RooRealVar *>(a)) { | |
| if (!v->isConstant()) | |
| allRooMultiPdfParams.add(*v); | |
| } else if(auto *cv = dynamic_cast<RooCategory *>(a)) { | |
| if (!cv->isConstant()) | |
| allRooMultiPdfParams.add(*cv); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/CascadeMinimizer.cc around lines 403 to 412, the loop only collects
RooRealVar parameters from each RooMultiPdf but omits RooCategory parameters;
update the loop to also check for dynamic_cast<RooCategory*> and add
non-constant categories to allRooMultiPdfParams (similar to how RooRealVar is
handled) so both numeric and categorical parameters are included when building
the RooArgSet.
3 participants
This functionality is intended to be upstreamed to RooFit, and therefore
refactored in one portable function that can be moved to the RooFit
helpers.
The major change is that the function that freezes the parameters now
returns a set with all parameters where the constant change was toggled.
This allows us to easily un-freeze these parameters later, without
having to carry around in the global state the full list of original
floating parameters.
Summary by CodeRabbit