Adding option to run expected limits for mu!=0 #1128
Conversation
Added option (`--signalStrengthForExpected`) which can be set to values
for which the expected limit and bands should be calculated.
* Observed value doesn't change (still assume pb is dervied wrt mu=0)
* setting `--signalStrengthForExpected 0` should yield identical
results compared to leaving default but might be slower as this
forces a recreation of the Asimov dataset
WalkthroughAdds a configurable non-standard Asimov mode via a new static signal-strength parameter and internal flag, extends the Asimov dataset API with an overwrite toggle, threads non-standard Asimov handling through expected-limit control flow, and registers a CLI option to set the signal strength. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant AL as AsymptoticLimits
participant WS as RooWorkspace
participant MCS as ModelConfig(s)
participant MCB as ModelConfig(b)
CLI->>AL: parse Options (--signalStrengthForExpected = muA)
AL->>AL: signalStrengthForExpected_ = muA
AL->>AL: doNonStandardAsimov_ = (muA != 0.0)
rect rgba(220,235,255,0.25)
note right of AL: Asimov dataset creation / reuse
AL->>AL: asimovDataset(WS, MCS, MCB, data, overwrite)
alt overwrite==false and cached
AL-->>CLI: return cached Asimov
else
AL->>AL: decide standard vs non-standard (doNonStandardAsimov_)
AL->>WS: build Asimov with mu = signalStrengthForExpected_
AL-->>CLI: return new Asimov (name includes mode & mu)
end
end
rect rgba(235,255,220,0.25)
note right of AL: Expected-limit computation flow
AL->>AL: runLimitExpected()
alt doNonStandardAsimov_ == true
AL->>AL: compute q(mu) using non-standard Asimov (q_At_muA, q_At_0)
AL->>AL: derive pb_expected from non-standard q values
else
AL->>AL: compute expected limit with standard Asimov (mu=0)
end
AL-->>CLI: return expected CLs/limit
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)
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}
| //static int minimizerStrategy_; | ||
|
|
||
| static double rValue_; | ||
| static double signalStrengthForExpected_; |
There was a problem hiding this comment.
warning: invalid case style for variable 'signalStrengthForExpected_' [readability-identifier-naming]
| static double signalStrengthForExpected_; | |
| static double signalStrengthForExpected; |
| double readMU_; | ||
| bool doCLs_; | ||
|
|
||
| bool doNonStandardAsimov_; |
There was a problem hiding this comment.
warning: invalid case style for private member 'doNonStandardAsimov_' [readability-identifier-naming]
| bool doNonStandardAsimov_; | |
| bool m_doNonStandardAsimov; |
src/AsymptoticLimits.cc
Outdated
| } | ||
|
|
||
| if (vm.count("signalStrengthForExpected") && !vm["signalStrengthForExpected"].defaulted()) { | ||
| doNonStandardAsimov_ = true; |
There was a problem hiding this comment.
warning: redundant boolean literal in conditional assignment [readability-simplify-boolean-expr]
src/AsymptoticLimits.cc:94:
- if (vm.count("signalStrengthForExpected") && !vm["signalStrengthForExpected"].defaulted()) {
- doNonStandardAsimov_ = true;
- } else doNonStandardAsimov_ = false;
+ doNonStandardAsimov_ = vm.count("signalStrengthForExpected") && !vm["signalStrengthForExpected"].defaulted();| r->setConstant(false); | ||
|
|
||
| double qMuAsimov = q_At_0-q_At_muA; | ||
| double N_for_expected = N+ROOT::Math::sqrt(qMuAsimov); |
There was a problem hiding this comment.
warning: no header providing "ROOT::Math::sqrt" is directly included [misc-include-cleaner]
src/AsymptoticLimits.cc:3:
- #include <RooRealVar.h>
+ #include <Math/Math.h>
+ #include <RooRealVar.h>| // std::cout << " --> this gives pb = " << pb_expected << " (N=" << N_for_expected << ")" << std::endl; | ||
| } | ||
|
|
||
| double errorlevel = 0.5 * pow(N+ROOT::Math::normal_quantile_c((doCLs_ ? pb_expected:1.)*(1-cl),1.0), 2); |
There was a problem hiding this comment.
warning: no header providing "ROOT::Math::normal_quantile_c" is directly included [misc-include-cleaner]
double errorlevel = 0.5 * pow(N+ROOT::Math::normal_quantile_c((doCLs_ ? pb_expected:1.)*(1-cl),1.0), 2);
^| asimovDataset_->SetName("_Asymptotic_asimovDataset_"); | ||
| asimovDataset_ = (noFitAsimov_ ? asimovutils::asimovDatasetNominal(mc_s, signalStrengthForExpected_, verbose) : | ||
| asimovutils::asimovDatasetWithFit(mc_s, data, snapGlobalObsAsimov,!bypassFrequentistFit_, signalStrengthForExpected_, verbose)); | ||
| asimovDataset_->SetName(Form("_Asymptotic_asimovDataset_%d_%g",doNonStandardAsimov_,signalStrengthForExpected_)); // in case we want to keep multiple asimov datasets in the same workspace |
There was a problem hiding this comment.
warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]
asimovDataset_->SetName(Form("_Asymptotic_asimovDataset_%d_%g",doNonStandardAsimov_,signalStrengthForExpected_)); // in case we want to keep multiple asimov datasets in the same workspace
^![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/AsymptoticLimits.cc (1)
160-165: Consider using RAII for exception safety.The temporary modification of
signalStrengthForExpected_could leave it in an incorrect state if an exception is thrown between setting and restoring. Consider using a scope guard or RAII wrapper.Example pattern:
struct SignalStrengthGuard { double& ref; double saved; SignalStrengthGuard(double& r, double newVal) : ref(r), saved(r) { r = newVal; } ~SignalStrengthGuard() { ref = saved; } }; SignalStrengthGuard guard(signalStrengthForExpected_, 0.0); RooAbsData &asimov = *asimovDataset(w, mc_s, mc_b, data, /*overwrite=*/doNonStandardAsimov_); // guard automatically restores on scope exit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
interface/AsymptoticLimits.h(3 hunks)src/AsymptoticLimits.cc(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
interface/AsymptoticLimits.h (1)
src/AsymptoticLimits.cc (2)
asimovDataset(769-795)asimovDataset(769-769)
src/AsymptoticLimits.cc (1)
src/AsimovUtils.cc (4)
asimovDatasetNominal(17-36)asimovDatasetNominal(17-17)asimovDatasetWithFit(38-177)asimovDatasetWithFit(38-38)
⏰ 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: review
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_2_0_pre2_ROOT632 - ROOT 6.32.06
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: precheckin
🔇 Additional comments (11)
interface/AsymptoticLimits.h (3)
51-51: LGTM!The new static member for signal strength configuration is well-placed and follows the existing pattern for static configuration members.
70-70: LGTM!The
overwriteparameter addition is backward compatible with its default value offalse, maintaining existing behavior when not explicitly specified.
79-80: LGTM!The new private flag to track non-standard Asimov mode is appropriately scoped as a private member.
src/AsymptoticLimits.cc (8)
40-40: LGTM!Static member initialization follows the existing pattern and correctly defaults to 0.0 (background-only).
50-50: LGTM!The CLI option is properly configured with a clear description that matches the PR objectives.
94-97: LGTM!The logic correctly identifies when non-standard Asimov mode is active based on whether the option was explicitly provided (even if set to 0), aligning with the PR description that setting it to 0 explicitly should force recreation.
395-395: LGTM!Correctly passes the overwrite flag to ensure the Asimov dataset is recreated with the appropriate signal strength for expected limits.
422-422: LGTM!The conditional correctly skips the warning when using non-standard Asimov, since a non-zero best fit is expected in that scenario.
503-503: LGTM!Correctly uses
pb_expected(computed from non-standard Asimov) instead ofpbin the error level calculation.
769-795: LGTM! Overwrite and reuse logic is sound.The implementation correctly:
- Reuses existing dataset when
!overwrite(line 776-778)- Passes
signalStrengthForExpected_to both Asimov creation methods (lines 789-790)- Creates unique dataset names including the flag and signal strength value (line 791)
This ensures that when using non-standard mode, each call (observed vs expected) creates its own Asimov with the correct μ value without unintended reuse.
477-501: Asymptotic non-standard Asimov formulas are correctly implemented.
qMuAsimov = q(0Asimov)−q(μAAsimov) gives the non-centrality λ, and
N_for_expected = N+√λ with pb_expected = Φ(N_for_expected) matches CCGV Eq.17 and CMS-NOTE 2011-005 Eq.37.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/AsymptoticLimits.cc (1)
474-498: Address missing include for ROOT::Math functions.The past review identified that no header providing
ROOT::Math::sqrtis directly included (line 495). Similarly,ROOT::Math::normal_quantile_c(line 500) lacks a direct include. As per the past review comments, please add the missing include.Apply this diff to add the missing header:
#include <RooRealVar.h> +#include <Math/Math.h> #include <RooArgSet.h>Based on past review comments.
🧹 Nitpick comments (2)
src/AsymptoticLimits.cc (2)
157-162: Consider refactoring to avoid temporary static member modification.The pattern of temporarily modifying
signalStrengthForExpected_is not thread-safe and introduces confusing side effects on static state. Consider refactoring to pass the signal strength value directly as a parameter toasimovDatasetinstead of relying on the static member, or ensure this code path is never executed concurrently.
788-788: Note: c-style vararg function usage.The past review flagged the use of
Form()as a c-style vararg function. While this is a minor code quality issue, you may want to consider using safer alternatives likestd::stringformatting in the future.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/part3/commonstatsmethods.md(1 hunks)src/AsymptoticLimits.cc(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/AsymptoticLimits.cc (1)
src/AsimovUtils.cc (4)
asimovDatasetNominal(17-36)asimovDatasetNominal(17-17)asimovDatasetWithFit(38-177)asimovDatasetWithFit(38-38)
🪛 markdownlint-cli2 (0.18.1)
docs/part3/commonstatsmethods.md
95-95: Spaces inside code span elements
(MD038, no-space-in-code)
⏰ 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: review
- GitHub Check: precheckin
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- 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: CMSSW_14_2_0_pre2_ROOT632 - ROOT 6.32.06
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
🔇 Additional comments (3)
src/AsymptoticLimits.cc (2)
40-40: LGTM!The static member initialization is consistent with the CLI option default value.
766-776: LGTM!The
overwriteparameter correctly controls whether to reuse the cached Asimov dataset. The logic ensures that whenoverwriteis false and a cached dataset exists, it's returned immediately, avoiding unnecessary recomputation.docs/part3/commonstatsmethods.md (1)
93-97: Documentation clearly explains the new feature.The documentation accurately describes the behavior of
--signalStrengthForExpectedand correctly notes that while setting it to 0 produces the same results as the default, it will be slower due to forcing Asimov dataset recreation. The clarification about the CLs denominator remaining defined with respect to μ=0 is also helpful.
| ``` | ||
| ## Expected limits assuming non-zero signal strengths | ||
|
|
||
| The median expected limit and quantiles can be calculated under Hypotheses other than at $\mu=0$. The Asymptotic expected limits can be calculated by setting the value `--signalStrengthForExpected mu0`. The Asimov dataset produced for the calculation will be generated for $\mu=$`mu0` instead of the usual $\mu=0$. Note that setting ` --signalStrengthForExpected 0` will give the same results as not setting the option, but will be slower as this forces a recreation of the Asimov dataset. |
There was a problem hiding this comment.
Fix markdown formatting: remove extra space in code span.
There's an extra space before the -- in the code span. This triggers a markdown linter warning.
Apply this diff:
-The median expected limit and quantiles can be calculated under Hypotheses other than at $\mu=0$. The Asymptotic expected limits can be calculated by setting the value `--signalStrengthForExpected mu0`. The Asimov dataset produced for the calculation will be generated for $\mu=$`mu0` instead of the usual $\mu=0$. Note that setting ` --signalStrengthForExpected 0` will give the same results as not setting the option, but will be slower as this forces a recreation of the Asimov dataset.
+The median expected limit and quantiles can be calculated under Hypotheses other than at $\mu=0$. The Asymptotic expected limits can be calculated by setting the value `--signalStrengthForExpected mu0`. The Asimov dataset produced for the calculation will be generated for $\mu=$`mu0` instead of the usual $\mu=0$. Note that setting `--signalStrengthForExpected 0` will give the same results as not setting the option, but will be slower as this forces a recreation of the Asimov dataset.As per static analysis hints.
📝 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.
| The median expected limit and quantiles can be calculated under Hypotheses other than at $\mu=0$. The Asymptotic expected limits can be calculated by setting the value `--signalStrengthForExpected mu0`. The Asimov dataset produced for the calculation will be generated for $\mu=$`mu0` instead of the usual $\mu=0$. Note that setting ` --signalStrengthForExpected 0` will give the same results as not setting the option, but will be slower as this forces a recreation of the Asimov dataset. | |
| The median expected limit and quantiles can be calculated under Hypotheses other than at $\mu=0$. The Asymptotic expected limits can be calculated by setting the value `--signalStrengthForExpected mu0`. The Asimov dataset produced for the calculation will be generated for $\mu=$`mu0` instead of the usual $\mu=0`. Note that setting `--signalStrengthForExpected 0` will give the same results as not setting the option, but will be slower as this forces a recreation of the Asimov dataset. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
95-95: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
In docs/part3/commonstatsmethods.md around line 95, there's an extra space
inside the inline code span before the option (` ` --signalStrengthForExpected
0` ), which triggers a markdown linter warning; remove the leading space so the
span reads `--signalStrengthForExpected 0` (update the single inline code
occurrence at that line to eliminate the extra space).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/AsymptoticLimits.cc (1)
94-96: Boolean expression can be simplified.As noted in a previous review, the conditional assignment can be simplified to a direct boolean expression.
🧹 Nitpick comments (1)
src/AsymptoticLimits.cc (1)
157-162: Consider optimizing the save/restore pattern.The code saves and restores
signalStrengthForExpected_even when it's unnecessary (whendoNonStandardAsimov_is false and the value is already 0.0). While the current implementation is correct, you could optimize by conditionally performing the save/restore:- // Bit of a waste of time if we are not using a non-standard value - double tmpsexp = signalStrengthForExpected_; - signalStrengthForExpected_ = 0.0; + double tmpsexp = 0.0; + if (doNonStandardAsimov_) { + tmpsexp = signalStrengthForExpected_; + signalStrengthForExpected_ = 0.0; + } RooAbsData &asimov = *asimovDataset(w, mc_s, mc_b, data, /*overwrite=*/doNonStandardAsimov_); - signalStrengthForExpected_ = tmpsexp; + if (doNonStandardAsimov_) { + signalStrengthForExpected_ = tmpsexp; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/AsymptoticLimits.cc(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/AsymptoticLimits.cc (1)
src/AsimovUtils.cc (4)
asimovDatasetNominal(17-36)asimovDatasetNominal(17-17)asimovDatasetWithFit(38-177)asimovDatasetWithFit(38-38)
⏰ 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: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_2_0_pre2_ROOT632 - ROOT 6.32.06
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: precheckin
- GitHub Check: review
🔇 Additional comments (6)
src/AsymptoticLimits.cc (6)
40-40: LGTM! Proper initialization of the new parameter.The static member is correctly initialized to 0.0, maintaining backward compatibility with the default background-only (mu=0) behavior.
50-50: LGTM! Clear and well-documented command-line option.The option is properly registered with an accurate description that explains the default behavior.
392-392: LGTM! Correct overwrite logic for expected limits.Passing
doNonStandardAsimov_as the overwrite parameter ensures the cached dataset is recreated when using a non-zero signal strength, preventing reuse of an incompatible cached dataset.
419-423: LGTM! Appropriate warning suppression.Correctly suppresses the warning when using non-standard Asimov datasets, since a non-zero best fit is expected and intentional in that case.
474-498: LGTM! Correct implementation of non-standard Asimov expected limits.The logic correctly computes the modified
pb_expectedvalue by:
- Evaluating the test statistic at mu=0 and mu=muA via constrained minimization
- Computing the difference to get the Asimov test statistic
- Adjusting the expected p-value using the asymptotic formula
The discrete-parameter-aware minimization (line 485-486, 489-490) is appropriately handled.
Note: Line 495 uses
ROOT::Math::sqrt, which has been flagged in a previous review for a missing header include.
766-792: LGTM! Correct caching logic for Asimov datasets.The changes properly handle caching and recreation of Asimov datasets:
- The
overwriteparameter controls whether to reuse the cached dataset- The
signalStrengthForExpected_value is correctly passed to both generation functions- The dataset name (line 788) includes both the flag and value, helping distinguish different parameter combinations
Notes:
- Line 788's use of
Form()has been flagged in previous reviews for c-style vararg usage- Line 773's caching strategy is correct but recreates datasets more often than strictly necessary when
doNonStandardAsimov_=true(both the mu=0 and mu!=0 datasets are recreated each run, even though the mu=0 dataset could be reused). This is a minor inefficiency acceptable for the current implementation.
|
I've been holding off on merging this as ideally we would have the same functionality to calculate these limits with toys. However, looking through The simplest would be to modify the HypoTestResult with a third sampling distribution that can be called if needed - i.e if calculating the expected upper limit assuming something other than We can merge this for now and possibly in the future, develop the |
|
@kcormi , perhaps something for a keen summer/PhD student to develop & validate? |
Added option (
--signalStrengthForExpected) which can be set to values for which the expected limit and bands should be calculated.--signalStrengthForExpected 0should yield identical results compared to leaving default but might be slower as this forces a recreation of the Asimov datasetSummary by CodeRabbit
New Features
Refactor
Documentation