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
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
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 computes RooArgSets of parameters to freeze via anonymous-namespace helpers and applies freezing locally. Combine.cc stops populating a global RooMultiPdf-parameter list. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Minimization Routine
participant CM as CascadeMinimizer (src)
participant Helper as freezeDiscParams() (anon ns)
participant Utils as utils::setAllConstant
Client->>CM: start minimize/improveOnce/hesse/minos
CM->>Helper: computeFrozenSet()
Helper-->>CM: RooArgSet frozenSet
CM->>Utils: setAllConstant(frozenSet, true)
CM->>CM: perform minimization step(s)
CM->>Utils: setAllConstant(frozenSet, false)
CM-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
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.
Summary by CodeRabbit