Add value clamping for RooRealVar parameters #1166

nucleosynthesis · 2025-11-05T13:19:34Z

Avoid error with silent clipping (make it explicit) when setting POI values in scan for upper limits.

Summary by CodeRabbit

Bug Fixes
- Parameter values are now clamped to valid bounds during evaluation, preventing out-of-range assignments and related unexpected behavior.
Tests
- Test suite updated to capture and compare command output against a reference file, improving output verification.

Avoid error with silent clipping (make it explicit) when setting POI values in scan for upper limits.

coderabbitai · 2025-11-05T13:19:54Z

Walkthrough

Clamp POI input values to the parameter's min/max before assignment in HybridNew::eval; update test CMake to redirect combine command output to a reference file for the LHC-limits test.

Changes

Cohort / File(s)	Summary
Parameter bounds clamping `src/HybridNew.cc`	In `eval`, clamp `rIn->getVal()` to `[r->getMin(), r->getMax()]` before setting the POI value; added comment noting the underflow case should not occur.
Test output redirection `test/CMakeLists.txt`	Replaced commented checkout/output handling with an active `CHECKOUT OUTPUT` and added `ALL_COMBINE_COMMANDS` entry to redirect combine stdout to `references/LHC-limits.out`.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Evaluation driver
    participant Hybrid as HybridNew::eval
    participant POI as POI parameter (r)

    rect rgb(235,245,255)
    Caller->>Hybrid: provide rIn (value)
    Hybrid->>POI: getMin(), getMax()
    note right of Hybrid: clamp value to [min,max]
    Hybrid->>POI: setVal(clamped_value)
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Small, localized logic change in src/HybridNew.cc (verify clamp semantics).
Minor CMake test change (verify test output redirection and reference file path).

Possibly related PRs

Automatically create script to recreate the reference files #1151 — Adjusts test/CMakeLists.txt to use ALL_COMBINE_COMMANDS/redirect combine output, related to the test change here.

Poem

🐰 I nibble values, trim each hop,

From min to max I gently stop,
No out-of-bounds shall cross my lawn,
Tests now log where outputs dawn,
A tidy hop — then on we yawn.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Add value clamping for RooRealVar parameters' accurately reflects the main change in src/HybridNew.cc, which adds clamping of POI parameter values to their bounds. However, the PR also modifies test/CMakeLists.txt to re-enable test output comparison, which is not mentioned in the title.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch nucleosynthesis-patch-1

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea6127a and c3cf686.

📒 Files selected for processing (1)

test/CMakeLists.txt (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 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.

📚 Learning: 2025-11-05T10:16:10.017Z

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.

Applied to files:

test/CMakeLists.txt

📚 Learning: 2025-11-05T10:12:07.843Z

Learnt from: guitargeek
Repo: cms-analysis/HiggsAnalysis-CombinedLimit PR: 1163
File: test/CMakeLists.txt:279-287
Timestamp: 2025-11-05T10:12:07.843Z
Learning: In PR #1163, the file test/references/hzz4l-significance.out was present and correctly included in the repository, despite an earlier incorrect comment flagging it as missing.

Applied to files:

test/CMakeLists.txt

⏰ 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). (11)

GitHub Check: dev3/latest - ROOT LCG master
GitHub Check: precheckin
GitHub Check: LCG_108 - ROOT 6.36.02
GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
GitHub Check: LCG_102 - ROOT 6.26.04
GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
GitHub Check: LCG_106 - ROOT 6.32.02
GitHub Check: Compile (py3.10, root6.26.4)
GitHub Check: Compile (py3.10, root6.32.2)
GitHub Check: Compile (py3.12, root6.34.4)

🔇 Additional comments (1)

test/CMakeLists.txt (1)

289-297: Re-enable LHC-limits test output comparison as requested.

The changes correctly re-enable output comparison for the LHC-limits test by uncommenting the CHECKOUT OUTPUT and set_property directives. The structure aligns with the established pattern used in similar tests like hzz4l-significance (lines 279–287) and LHC-Higgs-coupling-models-text2workspace (lines 270–277). The required reference file test/references/LHC-limits.out exists and contains valid combine output.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/HybridNew.cc (1)
716-718: Consider adding logging when clamping occurs for better observability.

The clamping logic is correct, but it operates silently. Since the PR aims to prevent "unexpected silent clipping," consider adding log messages when values are clamped. Additionally, line 717's comment states "this should never be the case"—if this condition truly shouldn't occur, consider adding a warning or assertion to catch unexpected behavior during development.

Apply this diff to add logging:
-		if ( rIn->getVal() > r->getMax() ) r->setVal(r->getMax());
-		else if ( rIn->getVal() < r->getMin() ) r->setVal(r->getMin()); // this should never be the case
-		else r->setVal(rIn->getVal());
+		if ( rIn->getVal() > r->getMax() ) {
+			if (verbose > 0) CombineLogger::instance().log("HybridNew.cc",__LINE__,std::string(Form("Clamping %s from %g to max %g", rIn->GetName(), rIn->getVal(), r->getMax())),__func__);
+			r->setVal(r->getMax());
+		}
+		else if ( rIn->getVal() < r->getMin() ) {
+			CombineLogger::instance().log("HybridNew.cc",__LINE__,std::string(Form("[WARNING] Unexpected: clamping %s from %g to min %g (this should not happen)", rIn->GetName(), rIn->getVal(), r->getMin())),__func__);
+			r->setVal(r->getMin());
+		}
+		else r->setVal(rIn->getVal());

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5e528b and ea6127a.

📒 Files selected for processing (1)

src/HybridNew.cc (1 hunks)

guitargeek · 2025-11-05T13:37:53Z

Hi, thanks a lot! Would you mind also enabling the comparison with the reference file for the test that was failing before with ROOT master?

It's just commenting these 4 lines back in and removing the TODO:
https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/test/CMakeLists.txt#L294

nucleosynthesis · 2025-11-05T13:59:56Z

Sure, just pushed that too

Add value clamping for RooRealVar parameters

ea6127a

Avoid error with silent clipping (make it explicit) when setting POI values in scan for upper limits.

nucleosynthesis mentioned this pull request Nov 5, 2025

Silent clipping when using --LHCmode LHC-limits --rMax 2.0 causes failures with ROOT master #1164

Closed

coderabbitai bot reviewed Nov 5, 2025

View reviewed changes

put back ref check for CAT23001 datacard

c3cf686

anigamova merged commit 723a5ee into main Nov 6, 2025
14 of 15 checks passed

nucleosynthesis deleted the nucleosynthesis-patch-1 branch November 6, 2025 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add value clamping for RooRealVar parameters #1166

Add value clamping for RooRealVar parameters #1166

Uh oh!

nucleosynthesis commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

guitargeek commented Nov 5, 2025

Uh oh!

nucleosynthesis commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add value clamping for RooRealVar parameters #1166

Add value clamping for RooRealVar parameters #1166

Uh oh!

Conversation

nucleosynthesis commented Nov 5, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Nov 5, 2025

Uh oh!

nucleosynthesis commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nucleosynthesis commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 5, 2025 •

edited

Loading