fix: histosys variations computed against sample nominal (#219)

[kratsg]

Summary

Fixes #219. The HistoSysModifier.apply was computing variations against the accumulated (already-modified) rates rather than the sample's nominal rates, violating the HistFactory formula:

lambda = (N + sum(delta_histosys(N))) * prod(kappa_multiplicative)

When a multiplicative modifier (e.g. normfactor) appeared before histosys in sample.modifiers, the histosys variation was computed against N * mu instead of N. Similarly, multiple histosys modifiers would chain against each other's output rather than all reference the same nominal.

Fix

Restructures _process_sample in HistFactoryDistChannel to use a two-pass approach:

• Pass 1: each additive modifier (histosys) is called with nominal_rates; we extract just the variation modifier.apply(ctx, nominal) - nominal and accumulate
• Pass 2: each multiplicative modifier (normfactor, normsys) is applied to nominal + sum(variations)

No changes to the Modifier ABC or individual modifier classes.

Also includes

• Backward-compatible pytensor.compile.maker/pytensor.compile.function import (try pytensor >= 3 path first, fall back to pytensor < 3)
• Corresponding mypy override and pixi environment pinned to pytensor 2.x for now

Test plan

• test_histosys_with_normfactor_uses_nominal -- normfactor-first ordering, alpha=0.5, verifies correct formula
• test_two_histosys_variations_sum_against_nominal -- two histosys on same sample both reference original nominal
• test_modifier_order_invariant -- [histosys, normfactor] vs [normfactor, histosys] produce identical results
• Full test suite: 914 passed, 3 skipped, 1 xfailed
• Pre-commit (ruff, mypy, codespell, etc.) all pass

Summary by CodeRabbit

• Bug Fixes

  • Corrected histogram factory modifier calculation so additive variations are computed against each sample’s nominal bin values before multiplicative modifiers are applied, preventing chained-scaling errors.

• Tests

  • Added regression tests validating modifier interpolation behavior across multiple modifier combinations and ordering.

• Chores

  • Updated type-checking/mypy configuration for compatibility with the newer runtime library.

[coderabbitai]

Warning

Rate limit exceeded

@kratsg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 49 minutes and 14 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f5b20442-6f53-41f9-8509-bd63ce9a1660

📥 Commits

Reviewing files that changed from the base of the PR and between 814323f and 8895d36.

📒 Files selected for processing (1)

• tests/test_histfactory.py

📝 Walkthrough

Walkthrough

HistFactory modifier application is refactored from sequential single-pass to two-pass logic to compute additive variations (histosys) against nominal rates before applying multiplicative modifiers, fixing order-dependent behavior. Regression tests validate the fix across modifier orderings. Mypy configuration updated for pytensor import compatibility.

Changes

HistFactory Modifier Order Bug Fix

Layer / File(s)	Summary
Core Modifier Logic Fix src/pyhs3/distributions/histfactory/__init__.py	_process_sample refactored to apply additive modifiers (histosys) in first pass by computing variations relative to nominal rates and summing, then apply multiplicative modifiers to combined (nominal + Σ additive) result. Docstring updated to document HistFactory prediction structure and nominal-based additive computation requirement.
Regression Tests tests/test_histfactory.py	New TestHistoSysNominalRates class validates histosys variations compute against sample nominal rates independent of modifier ordering, with test methods for normfactor-before-histosys sequences, multiple histosys modifiers on same sample, and expected-rate invariance under modifier reordering (including normsys).
Mypy Configuration pyproject.toml, src/pyhs3/compile.py	Mypy overrides updated to ignore pytensor.compile.function (removed in pytensor >= 3 and replaced by pytensor.compile.maker). The pytensor v2 fallback import's type-ignore annotation was narrowed.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main fix: histosys variations are now computed against sample nominal rates, directly addressing issue #219.
Linked Issues check	✅ Passed	The PR fully implements the requirements from issue #219: histosys modifiers now compute variations against nominal rates, accumulated additively, then multiplicative modifiers apply to the combined result, ensuring order-invariant behavior and correct formula adherence.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with fixing issue #219: the core logic change in _process_sample, supporting test cases, and backward-compatible pytensor import updates. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pixi.toml`:
- Line 27: Tests import the removed symbol directly (e.g. bare "from pytensor
import function") which will break under PyTensor 3.x; either pin PyTensor <3.0
in pyproject.toml to match pixi.toml or update each test
(test_histfactory_interpolations.py, test_histfactory.py, test_generic_parse.py,
test_functions.py, test_pdf.py, test_distributions.py) to guard/conditionalize
the import the same way src/ does (detect pytensor.__version__ or try/except and
import function from the correct submodule or provide a shim). Locate the bare
imports and replace them with the version-guarded import pattern used by the
source files, or add the upper bound to pyproject.toml to prevent installation
of pytensor>=3.0.

In `@tests/test_histfactory.py`:
- Around line 1460-1601: Extend the existing order-invariance test in
TestHistoSysNominalRates.test_modifier_order_invariant by adding a parallel case
that uses a multiplicative "normsys" modifier (instead of "normfactor") to
ensure normsys-before/after-histosys produce identical rates; create a
normsys_spec (type "normsys", same parameter name as the normfactor case) and
construct dist_ns_first = self._make_channel([normsys_spec, histosys_spec]) and
dist_sn_first = self._make_channel([histosys_spec, normsys_spec]), compute
expressions with _compute_expected_rates(...), build functions with
function([...], expr) and assert the two results are equal with
np.testing.assert_allclose (same pattern used for dist_hf_first/dist_nf_first).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0562fe72-18d6-493d-a13d-5f7ae4dff8c2

📥 Commits

Reviewing files that changed from the base of the PR and between 34a257b and fd0777b.

⛔ Files ignored due to path filters (1)

• pixi.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• pixi.toml
• pyproject.toml
• src/pyhs3/distributions/histfactory/__init__.py
• src/pyhs3/distributions/histfactory/modifiers.py
• src/pyhs3/model.py
• tests/test_histfactory.py
• tests/test_normalization.py
• tests/test_normalization_integration.py

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (402c0ec) to head (8895d36).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #220   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           39        39           
  Lines         2504      2509    +5     
  Branches       291       294    +3     
=========================================
+ Hits          2504      2509    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_histfactory.py`:
- Line 1523: Edit the comment text that reads "nominal=[10.0], histosys1 hi=[15]
lo=[5], histosys2 hi=[12] lo=[8], code0, both alpha=0.5." and remove or correct
the "code0" typo (replace with "alpha" or delete it) so the comment reads
consistently (e.g., "nominal=[10.0], histosys1 hi=[15] lo=[5], histosys2 hi=[12]
lo=[8], both alpha=0.5."). Ensure the updated comment is in the same test block
in tests/test_histfactory.py where that string appears.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ede92a5e-4f5f-4acc-894f-772a82f77403

📥 Commits

Reviewing files that changed from the base of the PR and between 4980bb3 and 814323f.

📒 Files selected for processing (1)

• tests/test_histfactory.py