feat: Add backend option to pyhf CLI

[matthewfeickert]

Description

Add ability to change the computational backend used by pyhf from the command line with a --backend flag.

Example:

$ curl -sL https://git.io/fjxXE | pyhf cls
{
    "CLs_exp": [
        0.07807427911686152,
        0.17472571775474582,
        0.35998495263681274,
        0.6343568235898907,
        0.8809947004472013
    ],
    "CLs_obs": 0.3599845631401913
}
$ curl -sL https://git.io/fjxXE | pyhf cls --backend pytorch
{
    "CLs_exp": [
        0.07808291912078857,
        0.1747395098209381,
        0.36000293493270874,
        0.6343730092048645,
        0.8810024857521057
    ],
    "CLs_obs": 0.3600029945373535
}
$ curl -sL https://git.io/fjxXE | pyhf cls --backend tensorflow
{
    "CLs_exp": [
        0.0780724585056305,
        0.17472276091575623,
        0.35998106002807617,
        0.6343533396720886,
        0.8809930086135864
    ],
    "CLs_obs": 0.35998106002807617
}

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

* Add backend option for the pyhf cls command line API
* Add tests to test_scripts.py to cover the pyhf cls backend option
* Answer the FAQ on setting the backend at the CLI

[matthewfeickert]

This still has some work to do on it, as TensorFlow is now complaining that

...
  File "/home/mcf/Code/GitHub/pyhf/src/pyhf/utils.py", line 162, in pvals_from_teststat
    if sqrtqmu_v < sqrtqmuA_v:
  File "/home/mcf/venvs/pyhf-dev/lib/python3.6/site-packages/tensorflow/python/framework/ops.py", line 690, in __bool__
    raise TypeError("Using a `tf.Tensor` as a Python `bool` is not allowed. "
TypeError: Using a `tf.Tensor` as a Python `bool` is not allowed. Use `if t is not None:` instead of `if t:` to test if a tensor is defined, and use TensorFlow ops such as tf.cond to execute subgraphs conditioned on the value of a tensor.

(tf.cond reference)

though I'm not sure why it hasn't picked this up before. To fix that requires replacing

https://github.com/diana-hep/pyhf/blob/f3b0680c115ef64cfb61a7631d9a550ec50c095f/src/pyhf/utils.py#L158-L169

with something along the lines of

    if not qtilde:  # qmu
        nullval = sqrtqmu_v
        altval = -(sqrtqmuA_v - sqrtqmu_v)
    else:  # qtilde
        def _sqrtqmu_v_case():
            nullval = sqrtqmu_v
            altval = -(sqrtqmuA_v - sqrtqmu_v)
            return nullval, altval
        
        def _sqrtqmuA_v_case():
            qmu = tensorlib.power(sqrtqmu_v, 2)
            qmu_A = tensorlib.power(sqrtqmuA_v, 2)
            nullval = (qmu + qmu_A) / (2 * sqrtqmuA_v)
            altval = (qmu - qmu_A) / (2 * sqrtqmuA_v)
            return nullval, altval
        
        import tensorflow as tf
        
        nullval, altval = tf.cond(
            tf.math.less(sqrtqmu_v[0], sqrtqmuA_v[0]), _sqrtqmu_v_case, _sqrtqmuA_v_case
        )

but to make that backend agnostic is going to take sometime (and I need some sleep).

TODO:

• Unify backed tensor comparison done in pvals_from_teststat
• Add tests of the cls CLI for different backends

[coveralls]

Coverage increased (+0.01%) to 93.574% when pulling c206a7e on feature/add-cli-backend-switching into f3b0680 on master.

[matthewfeickert]

As I've answered our only FAQ posted at the moment I should also add another before this goes in.

[kratsg]

I'm generally ok with these changes, but I don't think we should be supporting conditionals -- especially as the only place that is being used is for single-valued numbers which should be done in numpy only (and we're going to remove that code in the future).

[matthewfeickert]

I'm generally ok with these changes, but I don't think we should be supporting conditionals

Can you elaborate on why?

-- especially as the only place that is being used is for single-valued numbers

So you just want to have one line in pvals_from_teststat that is something like

nullvl, atlval = true_callable() if predicate else false_callable()

?

which should be done in numpy only

Why in numpy? Shouldn't this be backend independent?

(and we're going to remove that code in the future).

I've been defocused for the last months. What is getting removed and why?

[matthewfeickert]

@kratsg The discussion RE: conditional above can probably get moved to PR #535 as I want to break this out into a few PRs.

[matthewfeickert]

Given @kratsg's suggestion, this PR is going to change to promote the backend option to be a pyhf option instead of a pyhf cls option.

[kratsg]

Does this need to stay as a draft?

[matthewfeickert]

Does this need to stay as a draft?

Yes, because as we agreed to not do PR #535 and PR #537 this is being blocked by PR #541. So it isn't ready for review and should stay as a draft.

[lukasheinrich]

I think this would come after #583. Right now I don't feel too confident in the non-numpy backend to successfuly do the fits tbh

[lukasheinrich]

similar #535 I would vote to reopen this because we have a path to make the non-np backends fit robustly & reproduce the sbottom results

[codecov]

Codecov Report

Merging #534 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   94.99%   95.01%   +0.02%     
==========================================
  Files          47       47              
  Lines        2678     2689      +11     
  Branches      369      370       +1     
==========================================
+ Hits         2544     2555      +11     
  Misses         88       88              
  Partials       46       46

Flag	Coverage Δ
#unittests	95.01% <100%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
src/pyhf/cli/stats.py	97.67% <100%> (+0.45%)	⬆️
src/pyhf/utils.py	97.22% <100%> (+0.1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b30c6d1...7e168fa. Read the comment docs.

[lukasheinrich]

@matthewfeickert do you still want to add this?

[matthewfeickert]

Do you still want to add this?

Sorry I missed this. Yes, I think this should still go in and not get closed, though we need to decide if we will accept it like it is (with modifications necessary for merge) or if we should do PR #541 first instead.

[lukasheinrich]

i would do this now and not wait for the TF2 migration

[kratsg]

i would do this now and not wait for the TF2 migration

You and I have the same thought :)