Drop systematic lines that do not affect any channel/process when cal…#1238
Drop systematic lines that do not affect any channel/process when cal…#1238anigamova merged 2 commits intocms-analysis:mainfrom
Conversation
…ling combineCards with --include-channel or --exclude-channel
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a pruning step that computes systematics with no effect by comparing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/combineCards.py`:
- Around line 395-399: The current removed_systs comprehension treats any non
"-" value as an active effect but misses lnN nuisances that are neutral (e.g.
"1", "1.0", "1.0/1.0"), so change the predicate in the removed_systs set
comprehension to treat these as no-ops: for each (name, (pdf, pdfargs, effect,
nofloat)) in systlines, retrieve val = effect.get(b, {}).get(p, "-") and
consider it inactive if val == "-" or (pdf == "lnN" and both numerator and
denominator parse to float(1.0) after splitting on "/") (also treat single
numeric "1" or "1.0" as neutral); only mark a syst as active if any such val is
not inactive. Update the comprehension referencing removed_systs, systlines,
pdf, effect and keyline accordingly.
| # Remove systematics that don't affect any process/channel in the combination | ||
| removed_systs = { | ||
| name for name, (pdf, pdfargs, effect, nofloat) in systlines.items() | ||
| if not any(effect.get(b, {}).get(p, "-") != "-" for b, p, s in keyline) | ||
| } |
There was a problem hiding this comment.
Pruning condition misses neutral lnN effects (and keeps no-op nuisances).
At Line 398, a nuisance is considered active if value is not "-", but for lnN a value of 1.0 (or 1.0/1.0) is also null-effect. This means some systematics that do not affect retained content will not be pruned.
💡 Proposed fix
+# DatacardParser null-effect semantics:
+# - "-" -> 0.0 (null)
+# - lnN: 1.0 (or 1.0/1.0) is also null
+def _has_non_null_effect(pdf, effect, keyline):
+ for b, p, _ in keyline:
+ v = effect.get(b, {}).get(p, "-")
+ if v == "-":
+ continue
+ if pdf == "lnN":
+ try:
+ if "/" in v:
+ lo, hi = [float(x) for x in v.split("/", 1)]
+ if lo == 1.0 and hi == 1.0:
+ continue
+ elif float(v) == 1.0:
+ continue
+ except ValueError:
+ pass
+ return True
+ return False
+
removed_systs = {
name for name, (pdf, pdfargs, effect, nofloat) in systlines.items()
- if not any(effect.get(b, {}).get(p, "-") != "-" for b, p, s in keyline)
+ if not _has_non_null_effect(pdf, effect, keyline)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/combineCards.py` around lines 395 - 399, The current removed_systs
comprehension treats any non "-" value as an active effect but misses lnN
nuisances that are neutral (e.g. "1", "1.0", "1.0/1.0"), so change the predicate
in the removed_systs set comprehension to treat these as no-ops: for each (name,
(pdf, pdfargs, effect, nofloat)) in systlines, retrieve val = effect.get(b,
{}).get(p, "-") and consider it inactive if val == "-" or (pdf == "lnN" and both
numerator and denominator parse to float(1.0) after splitting on "/") (also
treat single numeric "1" or "1.0" as neutral); only mark a syst as active if any
such val is not inactive. Update the comprehension referencing removed_systs,
systlines, pdf, effect and keyline accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
=======================================
Coverage 20.89% 20.89%
=======================================
Files 195 195
Lines 26310 26310
Branches 3947 3947
=======================================
Hits 5498 5498
Misses 20812 20812 🚀 New features to boost your workflow:
|
|
failure in |
|
I see, we should still investigate, but not related to this PR. For this PR, could you please lint with the latest black? |
That seems to be coming from this root-project/root#13593, Jonas is already on it |
2 participants
…ling combineCards.py with --include-channel
As stated in the title, combineCards.py doesn't drop systematics that do not affect any channel/process when keeping only a subset of them (i.e. using
--include-channel. This makes the output datacard noisy if one wants to run a simple test with a subset of channels.I tried this with the Higgs observation model and the following command:
Running a simple fit on the output datacard
gives the same results before and after the change, as expected.
Summary by CodeRabbit