Check if either of BayesianSimple / MarkovChainMC#1244
Open
nucleosynthesis wants to merge 1 commit intomainfrom
Open
Check if either of BayesianSimple / MarkovChainMC#1244nucleosynthesis wants to merge 1 commit intomainfrom
nucleosynthesis wants to merge 1 commit intomainfrom
Conversation
Throw error if either are being used with discrete parameters, as currently this won't make any sense in combine.
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Combine.cc (1)
1203-1256:⚠️ Potential issue | 🟡 MinorFix clang-format issues flagged by CI.
The implementation correctly tracks discrete parameter detection through both the explicit
discreteParamspath and thepdfindexfallback path. The CI pipeline reports formatting mismatches in this function - please runclang-formatto resolve them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Combine.cc` around lines 1203 - 1256, The function Combine::addDiscreteNuisances has clang-format style mismatches; run clang-format on this function and fix whitespace/indentation to match project style (e.g., align braces, spaces around operators, consistent indentation inside the for/if blocks and around method calls). Reformat occurrences inside Combine::addDiscreteNuisances where CascadeMinimizerGlobalConfigs::O().pdfCategories, pdfindex fallback branch, the loop over clients, and the nested loops that build allRooMultiPdfParams are defined so the file compiles with CI formatting checks; ensure lines with CombineLogger::instance().log and Form(...) follow the repository's formatting rules. After formatting, stage the changes for this file only and re-run CI.
🧹 Nitpick comments (1)
src/Combine.cc (1)
810-812: Consider improving the error message for accuracy.The error message suggests removing
ADD_DISCRETE_FALLBACK, but looking at the logic inaddDiscreteNuisances(line 1215),hasDiscretebecomes true if any category is non-constant, regardless of whetherADD_DISCRETE_FALLBACKis set. The suggestion to "remove runtime def option ADD_DISCRETE_FALLBACK" only helps in specific edge cases.A clearer message might be:
throw std::invalid_argument( "Cannot currently use discrete parameters with Bayesian methods. " "Either freeze all discrete parameters to constant, or use a different method.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Combine.cc` around lines 810 - 812, The error message thrown in Combine.cc when (algo->name() == "BayesianSimple" || algo->name() == "MarkovChainMC") && wsHasDiscretes is misleading because wsHasDiscretes (set via addDiscreteNuisances/hasDiscrete) can be true independent of ADD_DISCRETE_FALLBACK; update the std::invalid_argument text to accurately reflect the condition (mention freezing discrete parameters or choosing a different algorithm) and remove the suggestion to "remove runtime def option ADD_DISCRETE_FALLBACK"; change the message near the check that uses algo->name(), BayesianSimple, MarkovChainMC, and wsHasDiscretes to the clearer wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Combine.cc`:
- Around line 807-812: The block around addDiscreteNuisances(w) and the Bayesian
check is failing clang-format; reformat the if-statement and surrounding lines
(the call to addDiscreteNuisances, the wsHasDiscretes variable and the if that
checks algo->name() == "BayesianSimple" || algo->name() == "MarkovChainMC") to
match project style—run clang-format on src/Combine.cc or manually adjust
spacing/brace placement so the throw std::invalid_argument line and the if
condition adhere to the codebase's formatting rules.
---
Outside diff comments:
In `@src/Combine.cc`:
- Around line 1203-1256: The function Combine::addDiscreteNuisances has
clang-format style mismatches; run clang-format on this function and fix
whitespace/indentation to match project style (e.g., align braces, spaces around
operators, consistent indentation inside the for/if blocks and around method
calls). Reformat occurrences inside Combine::addDiscreteNuisances where
CascadeMinimizerGlobalConfigs::O().pdfCategories, pdfindex fallback branch, the
loop over clients, and the nested loops that build allRooMultiPdfParams are
defined so the file compiles with CI formatting checks; ensure lines with
CombineLogger::instance().log and Form(...) follow the repository's formatting
rules. After formatting, stage the changes for this file only and re-run CI.
---
Nitpick comments:
In `@src/Combine.cc`:
- Around line 810-812: The error message thrown in Combine.cc when (algo->name()
== "BayesianSimple" || algo->name() == "MarkovChainMC") && wsHasDiscretes is
misleading because wsHasDiscretes (set via addDiscreteNuisances/hasDiscrete) can
be true independent of ADD_DISCRETE_FALLBACK; update the std::invalid_argument
text to accurately reflect the condition (mention freezing discrete parameters
or choosing a different algorithm) and remove the suggestion to "remove runtime
def option ADD_DISCRETE_FALLBACK"; change the message near the check that uses
algo->name(), BayesianSimple, MarkovChainMC, and wsHasDiscretes to the clearer
wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b964854e-e9d6-4ff7-b6ee-3ba8e040248f
📒 Files selected for processing (2)
interface/Combine.hsrc/Combine.cc
Comment on lines
+807
to
+812
| bool wsHasDiscretes = addDiscreteNuisances(w); | ||
|
|
||
| // Add a check that we're not trying to use discrete profiling with Bayesian methods | ||
| if ((algo->name() == "BayesianSimple" || algo->name() == "MarkovChainMC") && wsHasDiscretes){ | ||
| throw std::invalid_argument("Cannot currently use discrete parameters with Bayesian methods. Either remove discrete parameters or remove runtime def option ADD_DISCRETE_FALLBACK"); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix clang-format issues flagged by CI.
The logic correctly prevents Bayesian methods from being used with discrete parameters. However, the CI pipeline reports clang-format failures in this block. Please run clang-format to fix the formatting.
🔧 Suggested formatting fix
// Setup the CascadeMinimizer with discrete nuisances
- bool wsHasDiscretes = addDiscreteNuisances(w);
-
- // Add a check that we're not trying to use discrete profiling with Bayesian methods
- if ((algo->name() == "BayesianSimple" || algo->name() == "MarkovChainMC") && wsHasDiscretes){
- throw std::invalid_argument("Cannot currently use discrete parameters with Bayesian methods. Either remove discrete parameters or remove runtime def option ADD_DISCRETE_FALLBACK");
- }
+ bool wsHasDiscretes = addDiscreteNuisances(w);
+ // Add a check that we're not trying to use discrete profiling with Bayesian methods
+ if ((algo->name() == "BayesianSimple" || algo->name() == "MarkovChainMC") && wsHasDiscretes) {
+ throw std::invalid_argument(
+ "Cannot currently use discrete parameters with Bayesian methods. Either remove discrete "
+ "parameters or remove runtime def option ADD_DISCRETE_FALLBACK");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Combine.cc` around lines 807 - 812, The block around
addDiscreteNuisances(w) and the Bayesian check is failing clang-format; reformat
the if-statement and surrounding lines (the call to addDiscreteNuisances, the
wsHasDiscretes variable and the if that checks algo->name() == "BayesianSimple"
|| algo->name() == "MarkovChainMC") to match project style—run clang-format on
src/Combine.cc or manually adjust spacing/brace placement so the throw
std::invalid_argument line and the if condition adhere to the codebase's
formatting rules.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1244 +/- ##
==========================================
+ Coverage 20.89% 20.90% +0.01%
==========================================
Files 195 195
Lines 26310 26316 +6
Branches 3947 3947
==========================================
+ Hits 5498 5502 +4
- Misses 20812 20814 +2
🚀 New features to boost your workflow:
|
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
1 participant
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.
Throw error if either are being used with discrete parameters, as currently this won't make any sense in combine.
Summary by CodeRabbit
Bug Fixes