pyhf
pyhf copied to clipboard
feat: Experimental implementation of custom modifiers
Pull Request Description
Refer to #850.
Checklist Before Requesting Reviewer
- [ ] Tests are passing
- [ ] "WIP" removed from the title of the pull request
- [x] Selected an Assignee for the PR to be responsible for the log summary
Before Merging
For the PR Assignees:
- [x] Summarize commit messages into a comprehensive review of the PR
* Add `pyhf.experimental` module to allow for user defined custom modifiers
using numexpr string expressions.
* Add 'experimental' extra with numexpr dependency.
* Add tests for `pyhf.experimental.modifiers`.
* Add `pyhf.experimental` to API docs.
* Add 'experimental' extra to Docker image build.
Codecov Report
Attention: 10 lines in your changes are missing coverage. Please review.
Comparison is base (
ecbe778) 98.28% compared to head (bb302c2) 98.09%.
Additional details and impacted files
@@ Coverage Diff @@
## main #1991 +/- ##
==========================================
- Coverage 98.28% 98.09% -0.19%
==========================================
Files 69 70 +1
Lines 4538 4626 +88
Branches 803 814 +11
==========================================
+ Hits 4460 4538 +78
- Misses 45 51 +6
- Partials 33 37 +4
| Flag | Coverage Δ | |
|---|---|---|
| contrib | 97.68% <88.63%> (-0.18%) |
:arrow_down: |
| doctest | 61.13% <77.27%> (+0.42%) |
:arrow_up: |
| unittests-3.10 | 96.15% <88.63%> (-0.15%) |
:arrow_down: |
| unittests-3.11 | 96.15% <88.63%> (-0.15%) |
:arrow_down: |
| unittests-3.8 | 96.17% <88.63%> (-0.15%) |
:arrow_down: |
| unittests-3.9 | 96.19% <88.63%> (-0.15%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Files | Coverage Δ | |
|---|---|---|
| src/pyhf/experimental/modifiers.py | 88.63% <88.63%> (ø) |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
do we want to differentiate between experimental and contrib?
My reading of "contrib" is "quite stable API, but might get factored out eventually", while I would view "experimental" more like "anything goes, expect things to break". You could of course communicate how exactly you want the modules to be understood, but this is what I would think without further context.
do we want to differentiate between
experimentalandcontrib?
Yes, I think that's a good idea if we really mean "experimental". I have the same take as @alexander-held does.
Ok !
Related to the discussion in https://github.com/scikit-hep/cabinetry/issues/382, it may be good to provide a suitable schema for this as well (perhaps same PR makes sense?).
If I may add a request, it would be great if pyhf could catch a wrongly typed function expression in the custom modifier. I stumbled over srqt instead of sqrt in the related cabinetry issue discussion https://github.com/scikit-hep/cabinetry/issues/382#issuecomment-1362881632 which produced an obscure error output.
@kratsg while I will go back and review Issue #850 (I've forgotten all of it) can you also comment on what else would be needed for this PR before it would be ready for review?
Hi, just to comment also here that on the side of an ATLAS analysis there is some urgency with this and a merge would help us. We are looking for a way to incorporate different signal histograms into our statistical model and they scale differently with the parameter of interest, hence custom_modifiers are what we are looking for. Apart from that, we should be fine.
Hi, just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.
Cheers Javier
Hi, just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.
Cheers Javier
Relevant RTD build: https://pyhf--1991.org.readthedocs.build/en/1991/api.html#experimental
Small edge case: if you define a custom parameter to plug into add_custom_modifier and then define that parameter again in the "normal" way as a normfactor attached to a sample (like {"type": "normfactor", "name": "mu", "data": None} that seems to generally work but only as long as the bounds you put into the custom modifier version sync up with the bounds that the "normal" version would create by default. It probably would make sense to not add new bounds when the parameter is added as a normfactor.
While giving this another try, I ran into something that is either a bug or a usage question. I also have some API feedback. I opened #2350 for this to not clutter this PR too much.