pyhf icon indicating copy to clipboard operation
pyhf copied to clipboard

feat: Experimental implementation of custom modifiers

Open kratsg opened this issue 3 years ago • 14 comments

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.

kratsg avatar Sep 08 '22 15:09 kratsg

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.

codecov[bot] avatar Sep 08 '22 15:09 codecov[bot]

do we want to differentiate between experimental and contrib?

lukasheinrich avatar Sep 13 '22 15:09 lukasheinrich

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.

alexander-held avatar Sep 13 '22 21:09 alexander-held

do we want to differentiate between experimental and contrib?

Yes, I think that's a good idea if we really mean "experimental". I have the same take as @alexander-held does.

matthewfeickert avatar Sep 13 '22 21:09 matthewfeickert

Ok !

lukasheinrich avatar Sep 23 '22 07:09 lukasheinrich

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?).

alexander-held avatar Dec 20 '22 12:12 alexander-held

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.

rmnmllr avatar Dec 22 '22 15:12 rmnmllr

@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?

matthewfeickert avatar Jan 25 '23 17:01 matthewfeickert

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.

pariRieck avatar Jan 26 '23 17:01 pariRieck

Hi, just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.

Cheers Javier

jmontejo avatar Mar 27 '23 12:03 jmontejo

Hi, just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.

Cheers Javier

jmontejo avatar Mar 27 '23 12:03 jmontejo

Relevant RTD build: https://pyhf--1991.org.readthedocs.build/en/1991/api.html#experimental

matthewfeickert avatar Oct 10 '23 20:10 matthewfeickert

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.

alexander-held avatar Oct 11 '23 14:10 alexander-held

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.

alexander-held avatar Oct 13 '23 08:10 alexander-held