Skip to content

perf: remove dead work and vectorize loops in modifier construction#2715

Open
henryiii wants to merge 1 commit into
mainfrom
perf/modifier-build
Open

perf: remove dead work and vectorize loops in modifier construction#2715
henryiii wants to merge 1 commit into
mainfrom
perf/modifier-build

Conversation

@henryiii

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Part of the code review in #2706.

Summary

Performance and dead-code cleanups in the modifier construction layer. These touch numerical code, so the focus is correctness-preserving refactors rather than the (small) speedups.

  • shapesys: delete the never-read self.__shapesys_info tensor (a large (mods, samples, 3, bins) array built and immediately discarded; confirmed unused across src/ and tests/, including the name-mangled _shapesys_combined__shapesys_info).
  • shapesys / staterror: hoist default_backend.astensor(self._shapesys_mask) (resp. self._staterror_mask) out of the per-systematic _reindex_access_field loop so the full mask is tensorized once instead of once per systematic.
  • staterror.finalize: vectorize relerrs. The previous per-(sample, bin) Python comprehension with scalar indexing is replaced by default_backend array ops. A where-guarded denominator keeps bins with nomsall <= 0 at 0.0 and avoids emitting a divide-by-zero warning (pytest runs with filterwarnings=error). Verified numerically identical to the old logic, including the nomsall <= 0 edge case.
  • shapefactor: vectorize the access-field population. The element-wise triple loop is replaced by a per-(s, t) clipped gather plus np.where over batch_access < len(selection), preserving the exact else 0 fallback semantics.
  • normfactor.apply: hoist normfactors = self.param_viewer.get(pars) above the batch_size if/else (it was identical in both branches).
  • shapesys.apply: remove a duplicated back-to-back tensorlib, _ = get_backend().
  • paramview._precompute: build self.all_indices with dtype="int" to match self._all_indices (built with dtype="int"). Downstream int-casts in consumers are intentionally left in place to keep this PR contained. This change caused no backend test failures.

Test plan

Ran with both numpy and jax backends:

  • pytest tests/test_modifiers.py tests/test_pdf.py tests/test_constraints.py tests/test_validation.py tests/test_backend_consistency.py — all pass (97 passed, 6 skipped).
  • Broader fast suite pytest --ignore=tests/test_notebooks.py --ignore=tests/contrib --ignore=tests/benchmarks — 1114 passed. The only failures (7 tests in tests/test_infer.py, e.g. test_upper_limit*, test_toms748_scan) are a pre-existing RuntimeWarning: divide by zero in src/pyhf/infer/calculators.py:441, reproduced identically on clean main and unrelated to this PR.
  • Direct numerical equivalence check of the staterror relerrs vectorization against the original scalar comprehension (including a nomsall <= 0 bin): exact match.
  • prek -a (lint/format) clean.

🤖 Generated with Claude Code

Part of the code review in #2706.

- shapesys: delete the never-read __shapesys_info tensor
- shapesys/staterror: hoist the mask astensor() out of the per-systematic
  reindex loop
- staterror: vectorize relerrs (np.where-guarded division, no scalar
  comprehension, no divide-by-zero warning)
- shapefactor: vectorize the access-field population per (s, t) with a
  clipped gather + np.where fallback
- normfactor: hoist param_viewer.get(pars) above the batch if/else
- shapesys: drop a duplicate get_backend() call in apply
- paramview: build all_indices with dtype="int" to match _all_indices

Assisted-by: ClaudeCode:claude-opus-4.8
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@henryiii, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 29 minutes and 54 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 854877b4-4920-46ef-bb67-e641a5f85e5e

📥 Commits

Reviewing files that changed from the base of the PR and between 0e54a51 and 73aa8d0.

📒 Files selected for processing (5)
  • src/pyhf/modifiers/normfactor.py
  • src/pyhf/modifiers/shapefactor.py
  • src/pyhf/modifiers/shapesys.py
  • src/pyhf/modifiers/staterror.py
  • src/pyhf/parameters/paramview.py

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

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

@henryiii henryiii marked this pull request as ready for review June 12, 2026 17:56
@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.51%. Comparing base (0e54a51) to head (73aa8d0).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2715      +/-   ##
==========================================
- Coverage   98.28%   96.51%   -1.77%     
==========================================
  Files          65       65              
  Lines        4305     4309       +4     
  Branches      465      464       -1     
==========================================
- Hits         4231     4159      -72     
- Misses         46      118      +72     
- Partials       28       32       +4     
Flag Coverage Δ
contrib ?
doctest ?
unittests-3.10 ?
unittests-3.11 96.47% <100.00%> (+<0.01%) ⬆️
unittests-3.12 ?
unittests-3.13 ?
unittests-3.9 96.54% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant