perf: remove dead work and vectorize loops in modifier construction#2715
perf: remove dead work and vectorize loops in modifier construction#2715henryiii wants to merge 1 commit into
Conversation
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
|
Warning Review limit reached
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 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 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.
self.__shapesys_infotensor (a large(mods, samples, 3, bins)array built and immediately discarded; confirmed unused acrosssrc/andtests/, including the name-mangled_shapesys_combined__shapesys_info).default_backend.astensor(self._shapesys_mask)(resp.self._staterror_mask) out of the per-systematic_reindex_access_fieldloop so the full mask is tensorized once instead of once per systematic.relerrs. The previous per-(sample, bin)Python comprehension with scalar indexing is replaced bydefault_backendarray ops. Awhere-guarded denominator keeps bins withnomsall <= 0at0.0and avoids emitting a divide-by-zero warning (pytest runs withfilterwarnings=error). Verified numerically identical to the old logic, including thenomsall <= 0edge case.(s, t)clipped gather plusnp.whereoverbatch_access < len(selection), preserving the exactelse 0fallback semantics.normfactors = self.param_viewer.get(pars)above thebatch_sizeif/else (it was identical in both branches).tensorlib, _ = get_backend().self.all_indiceswithdtype="int"to matchself._all_indices(built withdtype="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).pytest --ignore=tests/test_notebooks.py --ignore=tests/contrib --ignore=tests/benchmarks— 1114 passed. The only failures (7 tests intests/test_infer.py, e.g.test_upper_limit*,test_toms748_scan) are a pre-existingRuntimeWarning: divide by zeroinsrc/pyhf/infer/calculators.py:441, reproduced identically on cleanmainand unrelated to this PR.staterrorrelerrsvectorization against the original scalar comprehension (including anomsall <= 0bin): exact match.prek -a(lint/format) clean.🤖 Generated with Claude Code