-
Notifications
You must be signed in to change notification settings - Fork 421
Add channel masks tests and CMSHistFunc test to the CMake configuration #1187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughExpanded MultiDimFit parameter ranges from r=-1,1 to r=-5,5 across CI and test steps, added CI workflow jobs that run text2workspace with channel masks and MultiDimFit on a new multi-region htt datacard, and enabled three CMSHistFunc-related tests in CMake. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test/references/cmshistfunc.outis excluded by!**/*.outtest/references/cmshistfunc_channel_masks.outis excluded by!**/*.outtest/references/cmshistfunc_shapeN.outis excluded by!**/*.out
📒 Files selected for processing (3)
.github/workflows/cvmfs-ci.yml(1 hunks)data/ci/htt_multiple_regions.txt(1 hunks)test/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: guitargeek
Repo: cms-analysis/HiggsAnalysis-CombinedLimit PR: 1163
File: test/CMakeLists.txt:270-277
Timestamp: 2025-11-05T10:16:10.042Z
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.
⏰ 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). (10)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
🔇 Additional comments (6)
.github/workflows/cvmfs-ci.yml (1)
222-227: Verify that the required input ROOT file is available in the workflow environment.The datacard file
htt_multiple_regions.txttypically requires a corresponding ROOT file (htt_multiple_regions.input.root) containing the histogram shapes. The test suite correctly copies both files (test/CMakeLists.txt lines 193–194), but this workflow step does not explicitly stage the input ROOT file. Verify that this file is either:
- Pre-staged in the Docker environment, or
- Should be explicitly copied/downloaded in this workflow step.
data/ci/htt_multiple_regions.txt (2)
1-95: Datacard structure and syntax are sound.The multi-region datacard is properly formatted with correct Combine syntax, comprehensive nuisance parameter definitions, and proper process/bin mappings across three regions.
1-10: The required input ROOT file is already committed to the repository.
data/ci/htt_multiple_regions.input.rootexists in the repository (242K, committed Dec 2 15:28). No action needed.test/CMakeLists.txt (3)
182-189: CMSHistFunc test case is properly configured.The
cmshistfunctest follows established patterns and correctly stages required input files.
192-199: CMSHistFunc with channel masks test is properly configured.The
cmshistfunc_channel_maskstest correctly:
- Stages both the datacard and input ROOT file
- Passes
--channel-masksflag to text2workspace.py (line 196)- Sets the channel mask parameter in MultiDimFit (line 198)
- Outputs to a distinct workspace file for clarity
Note: The filename used here (
htt_multiple_regions.txt) is correct, unlike the typo in the workflow file.
202-209: CMSHistFunc shapeN test case is properly configured.The
cmshistfunc_shapeNtest follows the established pattern and correctly stages required files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1187 +/- ##
==========================================
- Coverage 22.25% 20.69% -1.56%
==========================================
Files 195 195
Lines 26154 26154
Branches 3884 3923 +39
==========================================
- Hits 5820 5412 -408
- Misses 20334 20742 +408 see 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks to codecov report from #1137 I realized that we don't have tests with channel masks, hence this PR. But it looks like something change between 6.26 and 6.30, and with channel masking we arrive to different fit results. Everything with ROOT>=6.30 is consistent. TBU |
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.