Andreh7 ah 2015 06 03 more verbose parameter at boundary checking 102x#550
Open
nucleosynthesis wants to merge 4 commits into
Open
Conversation
…:isParameterAtLowerBoundary(..) and utils::isParameterAtUpperBoundary(..); method utils::anyParameterAtBoundaries(..) now prints more details about why a parameter is at a boundary
…d isParameterAtLowerBoundary(..) replaced by isParameterAtUpperBoundary(..)
… upper/lower/both boundaries more explicit
…ng' of git://github.com/andreh7/HiggsAnalysis-CombinedLimit into 102x
Member
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughAdds two new boundary-check helpers to interface/utils.h and refactors src/utils.cc to split lower/upper boundary checks, introduce a wrapper isParameterAtBoundary, update anyParameterAtBoundaries to use both checks, and enhance logging with boundary specifics and per-parameter hit tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Utils
Caller->>Utils: isParameterAtBoundary(param)
activate Utils
Utils->>Utils: isParameterAtLowerBoundary(param)
Utils-->>Utils: lowerHit (bool)
Utils->>Utils: isParameterAtUpperBoundary(param)
Utils-->>Utils: upperHit (bool)
Utils-->>Caller: lowerHit || upperHit
deactivate Utils
sequenceDiagram
participant Analyzer as anyParameterAtBoundaries
participant Utils
Analyzer->>Utils: isParameterAtLowerBoundary(p)
Utils-->>Analyzer: lowerHit
Analyzer->>Utils: isParameterAtUpperBoundary(p)
Utils-->>Analyzer: upperHit
Analyzer->>Analyzer: Log details, update counters
Analyzer-->>Analyzer: isBad = lowerHit || upperHit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
interface/utils.h (1)
113-118: Clarify API semantics and edge cases in the new boundary helpersThe additions look good. Please refine the docstrings to explicitly state:
- “One sigma” uses the RooRealVar’s asymmetric errors: errLo/errHi (with the lower treated as positive magnitude).
- Behavior when errors are zero or not available (e.g., return false and/or log). This matters because the implementation divides by the error.
Optionally, consider making the nSigma threshold configurable (defaulting to 1.0) if callers may want different strictness.
src/utils.cc (1)
995-1015: Minor: avoid double map lookups and unintended insertionoperator[] inserts keys; since we are inside isBad, it’s fine, but you still perform the lookup twice. Cache the count to avoid two lookups and make intent explicit.
- if( verbosity >= 9 || (timesFoundAtBoundary[varName] < 3 && verbosity > -1) ){ + const auto it = timesFoundAtBoundary.find(varName); + const unsigned char seen = (it == timesFoundAtBoundary.end()) ? 0 : it->second; + if (verbosity >= 9 || (seen < 3 && verbosity > -1)) { fprintf(CloseCoutSentry::trueStdOutGlobal()," [WARNING] Found [%s] at ", (*a).GetName()); ... } ... - timesFoundAtBoundary[varName]++; + timesFoundAtBoundary[varName] = seen + 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
interface/utils.h(1 hunks)src/utils.cc(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
interface/utils.h (1)
src/utils.cc (4)
isParameterAtLowerBoundary(942-957)isParameterAtLowerBoundary(942-942)isParameterAtUpperBoundary(959-974)isParameterAtUpperBoundary(959-959)
src/utils.cc (1)
src/CloseCoutSentry.cc (2)
trueStdOutGlobal(72-76)trueStdOutGlobal(72-72)
🔇 Additional comments (1)
src/utils.cc (1)
976-979: Wrapper is clear and correctThe wrapper composes the two checks as intended, improving readability for call sites.
Comment on lines
+942
to
+957
| bool utils::isParameterAtLowerBoundary( const RooRealVar ¶m ){ | ||
|
|
||
| double vMin = param.getMin(); | ||
| double vMax = param.getMax(); | ||
| double val = param.getVal(); | ||
| double errLo = -1.0 * param.getErrorLo(); | ||
| double errHi = param.getErrorHi(); | ||
|
|
||
| double pullMin = (val-vMin) / (errLo); | ||
|
|
||
| float nSigma=1.0; | ||
|
|
||
| if(pullMin < nSigma){ | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against division by zero/invalid errors in lower-boundary check
param.getErrorLo() can be 0 (or non-finite) when MINOS wasn’t run or errors are not available, which will cause a division by zero or NaN pulls. Also prefer consistent double precision and early returns.
Apply this refactor:
bool utils::isParameterAtLowerBoundary( const RooRealVar ¶m ){
double vMin = param.getMin();
double val = param.getVal();
- double errLo = -1.0 * param.getErrorLo();
-
- double pullMin = (val-vMin) / (errLo);
-
- float nSigma=1.0;
-
- if(pullMin < nSigma){
- return true;
- }
-
- return false;
+ // Use positive magnitude for the lower asymmetric error.
+ double sigmaLo = -param.getErrorLo();
+ // Fall back to symmetric error if needed.
+ if (!std::isfinite(sigmaLo) || sigmaLo <= 0.0) {
+ sigmaLo = param.getError();
+ }
+ // If we still don't have a valid error, we cannot assess proximity.
+ if (!std::isfinite(sigmaLo) || sigmaLo <= 0.0) {
+ return false;
+ }
+ constexpr double nSigma = 1.0;
+ const double pullMin = (val - vMin) / sigmaLo;
+ return pullMin < nSigma;
}📝 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.
Suggested change
| bool utils::isParameterAtLowerBoundary( const RooRealVar ¶m ){ | |
| double vMin = param.getMin(); | |
| double vMax = param.getMax(); | |
| double val = param.getVal(); | |
| double errLo = -1.0 * param.getErrorLo(); | |
| double errHi = param.getErrorHi(); | |
| double pullMin = (val-vMin) / (errLo); | |
| float nSigma=1.0; | |
| if(pullMin < nSigma){ | |
| return true; | |
| } | |
| return false; | |
| } | |
| bool utils::isParameterAtLowerBoundary(const RooRealVar ¶m) { | |
| double vMin = param.getMin(); | |
| double val = param.getVal(); | |
| // Use positive magnitude for the lower asymmetric error. | |
| double sigmaLo = -param.getErrorLo(); | |
| // Fall back to symmetric error if needed. | |
| if (!std::isfinite(sigmaLo) || sigmaLo <= 0.0) { | |
| sigmaLo = param.getError(); | |
| } | |
| // If we still don't have a valid error, we cannot assess proximity. | |
| if (!std::isfinite(sigmaLo) || sigmaLo <= 0.0) { | |
| return false; | |
| } | |
| constexpr double nSigma = 1.0; | |
| const double pullMin = (val - vMin) / sigmaLo; | |
| return pullMin < nSigma; | |
| } |
🤖 Prompt for AI Agents
In src/utils.cc around lines 942 to 957, the lower-boundary check divides by
param.getErrorLo() which can be zero or non-finite; change to use double
precision throughout, fetch double errorLo = param.getErrorLo(), and if errorLo
<= 0.0 or !std::isfinite(errorLo) immediately return false to avoid division by
zero/NaN; then compute pullMin = (val - vMin) / (-errorLo) (or equivalently use
errLo = -errorLo) and compare to a double nSigma = 1.0, returning true only when
pullMin < nSigma, otherwise false.
Comment on lines
+959
to
974
| bool utils::isParameterAtUpperBoundary( const RooRealVar ¶m ){ | ||
|
|
||
| double vMax = param.getMax(); | ||
| double val = param.getVal(); | ||
| double errHi = param.getErrorHi(); | ||
|
|
||
| double pullMax = (vMax-val) / (errHi); | ||
|
|
||
| float nSigma=1.0; | ||
|
|
||
| if(pullMin < nSigma || pullMax < nSigma){ | ||
| if(pullMax < nSigma){ | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Mirror the same robustness for upper-boundary check
Same division-by-zero risk for errHi. Align types and use early returns.
bool utils::isParameterAtUpperBoundary( const RooRealVar ¶m ){
double vMax = param.getMax();
double val = param.getVal();
- double errHi = param.getErrorHi();
-
- double pullMax = (vMax-val) / (errHi);
-
- float nSigma=1.0;
-
- if(pullMax < nSigma){
- return true;
- }
-
- return false;
+ double sigmaHi = param.getErrorHi();
+ // Fall back to symmetric error if needed.
+ if (!std::isfinite(sigmaHi) || sigmaHi <= 0.0) {
+ sigmaHi = param.getError();
+ }
+ if (!std::isfinite(sigmaHi) || sigmaHi <= 0.0) {
+ return false;
+ }
+ constexpr double nSigma = 1.0;
+ const double pullMax = (vMax - val) / sigmaHi;
+ return pullMax < nSigma;
}📝 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.
Suggested change
| bool utils::isParameterAtUpperBoundary( const RooRealVar ¶m ){ | |
| double vMax = param.getMax(); | |
| double val = param.getVal(); | |
| double errHi = param.getErrorHi(); | |
| double pullMax = (vMax-val) / (errHi); | |
| float nSigma=1.0; | |
| if(pullMin < nSigma || pullMax < nSigma){ | |
| if(pullMax < nSigma){ | |
| return true; | |
| } | |
| return false; | |
| } | |
| bool utils::isParameterAtUpperBoundary( const RooRealVar ¶m ){ | |
| double vMax = param.getMax(); | |
| double val = param.getVal(); | |
| double sigmaHi = param.getErrorHi(); | |
| // Fall back to symmetric error if needed. | |
| if (!std::isfinite(sigmaHi) || sigmaHi <= 0.0) { | |
| sigmaHi = param.getError(); | |
| } | |
| if (!std::isfinite(sigmaHi) || sigmaHi <= 0.0) { | |
| return false; | |
| } | |
| constexpr double nSigma = 1.0; | |
| const double pullMax = (vMax - val) / sigmaHi; | |
| return pullMax < nSigma; | |
| } |
🤖 Prompt for AI Agents
In src/utils.cc around lines 959 to 974, the upper-boundary check divides by
errHi without guarding against zero and uses mixed float/double types; update
the function to mirror the lower-boundary robustness by treating nSigma as a
double, checking if fabs(errHi) is below a small epsilon (e.g. 1e-12) and
returning false early to avoid division-by-zero, compute pullMax with doubles,
and keep the existing logic (return true when pullMax < nSigma) with the
early-return style.
Comment on lines
985
to
992
| RooLinkedListIter iter = params.iterator(); int i = 0; | ||
| for (RooRealVar *a = (RooRealVar *) iter.Next(); a != 0; a = (RooRealVar *) iter.Next(), ++i) { | ||
|
|
||
| bool isBad = isParameterAtBoundary(*a); | ||
|
|
||
| bool atLowerBoundary = isParameterAtLowerBoundary(*a); | ||
| bool atUpperBoundary = isParameterAtUpperBoundary(*a); | ||
| bool isBad = atLowerBoundary || atUpperBoundary; | ||
|
|
||
| if(isBad){ |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe C-style cast in iteration over RooArgSet can cause UB
The loop casts every element to RooRealVar* without a type check: if the set contains non-RooRealVar (e.g., RooCategory), dereferencing yields undefined behavior. Use RooAbsArg* + dynamic_cast and skip non-RooRealVar entries.
- RooLinkedListIter iter = params.iterator(); int i = 0;
- for (RooRealVar *a = (RooRealVar *) iter.Next(); a != 0; a = (RooRealVar *) iter.Next(), ++i) {
-
- bool atLowerBoundary = isParameterAtLowerBoundary(*a);
- bool atUpperBoundary = isParameterAtUpperBoundary(*a);
+ RooLinkedListIter iter = params.iterator(); int i = 0;
+ for (RooAbsArg *arg = (RooAbsArg *) iter.Next(); arg != 0; arg = (RooAbsArg *) iter.Next(), ++i) {
+ RooRealVar *a = dynamic_cast<RooRealVar *>(arg);
+ if (!a) continue;
+
+ const bool atLowerBoundary = isParameterAtLowerBoundary(*a);
+ const bool atUpperBoundary = isParameterAtUpperBoundary(*a);This aligns with iteration patterns used elsewhere in this file and prevents crashes.
🤖 Prompt for AI Agents
In src/utils.cc around lines 985-992, the loop unsafely C-casts elements from
the RooArgSet iterator to RooRealVar* which can yield undefined behavior if the
set contains non-RooRealVar entries; instead iterate using RooAbsArg* (e.g.,
RooAbsArg *arg = (RooAbsArg*) iter.Next()), then use
dynamic_cast<RooRealVar*>(arg) and skip (continue) when the cast returns nullptr
so only actual RooRealVar instances are processed; keep the existing boundary
checks and logic but operate on the safely casted RooRealVar*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Originally from #197, moved over to
102xbranch.Summary by CodeRabbit
New Features
Refactor