fix: Catch use of multi-component parameters as POI with error message

[alexander-held]

Description

Resolves #2200.

This addresses #2194. When a parameter with multiple components (shapefactor / shapesys / staterror) is used as POI, the model construction currently fails at an assert (assuming Python is not run with -O). This PR turns the failure into an exception with an error message to provide more context.

For simple MLEs, bypassing the assert actually makes things work (so one could argue to not catch this early and force the exception). However, this would cause even more confusing issues down the line once the POI starts mattering. As an example, a hypotest could result in RuntimeWarning: invalid value encountered in subtract within scipy. I do not see any benefit in allowing to bypass the exception to allow running things that do not depend on the POI definition though.

Checklist Before Requesting Reviewer

• Tests are passing
• "WIP" removed from the title of the pull request
• Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

• Summarize commit messages into a comprehensive review of the PR

(summary provided by @matthewfeickert)

* Raise exceptions.InvalidModel for multiple component parameter of interest.
  This guards against modifiers like 'shapefactor', 'shapesys', and 'staterror'
  from being used as POIs.
   - Remove use of 'assert' to check the same information.
* Add test to test_pdf.py to validate.

[alexander-held]

Not sure how to best handle custom modifiers: the model config contains the desired POI in parameters but not in modifiers (should it?). Working around this now by using a get, but unclear to me whether there may be a better approach.

Switched approach to be more generic.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (22c1699) 98.30% compared to head (87a5f5d) 98.30%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2197   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4533     4533           
  Branches      801      802    +1     
=======================================
  Hits         4456     4456           
  Misses         45       45           
  Partials       32       32           

Flag	Coverage Δ
contrib	97.88% <100.00%> (ø)
doctest	61.08% <33.33%> (-0.05%)	⬇️
unittests-3.10	96.31% <100.00%> (ø)
unittests-3.11	96.31% <100.00%> (ø)
unittests-3.8	96.33% <100.00%> (ø)
unittests-3.9	96.36% <100.00%> (ø)

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

Impacted Files	Coverage Δ
src/pyhf/pdf.py	98.03% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[alexander-held]

This now handles shapefactor / shapesys / staterror if they act on channels with multiple bins. All of these cases would fail the assert below. Setups like a histosys as POI seem to technically work, so even if they might not be useful in practice I guess there is no need to catch those here.

[matthewfeickert]

Thanks @alexander-held and LGTM. 👍 I just have one question on if we couldn't go further.

[matthewfeickert]

After the assert removal this all LGTM @alexander-held. 👍 If @kratsg also approves then once this is merged and I finish the other backports I'll release pyhf v0.7.2 with this in it today.