revert: Allow 0-dimensional shape tensors #972

matthewfeickert · 2020-07-21T07:48:05Z

Description

Resolves #954

This PR allows for tensors to take any shape, even empty () shapes. This effectively reverts PR #413. It also enforces that the optimizers return the objective function as a scalar (shape ()), resulting in the test statistic being a scalar.

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

* Allow for arbitrary shape tensors, including 0-dimensional ("empty") shape tensors (floats)
   - Reverts PR #413
* Enforce optimizers return objective function as a scalar to harmonize return type
* Revert tests that enforce minimum shape
* Add tests for return structure shape of pyhf.infer.mle.fit
* Add docstring examples for astensor

src/pyhf/tensor/pytorch_backend.py

matthewfeickert · 2020-07-21T09:56:19Z

@kratsg @lukasheinrich Still a few things to iron out, but feedback now is welcome.

src/pyhf/infer/test_statistics.py

matthewfeickert · 2020-07-21T17:56:02Z

The test_backend_consistency issues are currently due to JAX. It is currently treating 0-dimensional tensors differently from all the other backends. The problem is the test statistic should be a float, but we've been treating it like a tensor in the past.

matthewfeickert · 2020-07-21T19:43:32Z

This is missing tests for the atleast_1d. Will add later.

codecov · 2020-07-21T19:51:25Z

Codecov Report

Merging #972 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
- Coverage   96.65%   96.63%   -0.02%     
==========================================
  Files          59       59              
  Lines        3284     3265      -19     
  Branches      447      447              
==========================================
- Hits         3174     3155      -19     
  Misses         69       69              
  Partials       41       41

Flag	Coverage Δ
#unittests	`96.63% <100.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
src/pyhf/infer/mle.py	`100.00% <ø> (ø)`
src/pyhf/infer/test_statistics.py	`100.00% <ø> (ø)`
src/pyhf/optimize/opt_numpy.py	`100.00% <100.00%> (ø)`
src/pyhf/optimize/opt_pytorch.py	`100.00% <100.00%> (ø)`
src/pyhf/optimize/opt_tflow.py	`100.00% <100.00%> (ø)`
src/pyhf/tensor/jax_backend.py	`95.72% <100.00%> (-0.18%)`	⬇️
src/pyhf/tensor/numpy_backend.py	`98.21% <100.00%> (-0.08%)`	⬇️
src/pyhf/tensor/pytorch_backend.py	`98.03% <100.00%> (-0.08%)`	⬇️
src/pyhf/tensor/tensorflow_backend.py	`96.36% <100.00%> (-0.16%)`	⬇️

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 eadcd11...385a6f8. Read the comment docs.

src/pyhf/infer/test_statistics.py

matthewfeickert · 2020-07-21T23:27:38Z

This still has an annoying doctest issue, which is actually a problem, but I'll go ahead and let people start to review as this is mostly done.

matthewfeickert · 2020-07-21T23:38:11Z

Fixing the dotctest error with

diff --git a/src/pyhf/optimize/opt_scipy.py b/src/pyhf/optimize/opt_scipy.py
index 408754f6..eb551bbf 100644
--- a/src/pyhf/optimize/opt_scipy.py
+++ b/src/pyhf/optimize/opt_scipy.py
@@ -28,7 +28,7 @@ class scipy_optimizer(object):
 
         Returns:
             bestfit parameters
-        
+
         """
         fixed_vals = fixed_vals or []
         indices = [i for i, _ in fixed_vals]
@@ -49,5 +49,5 @@ class scipy_optimizer(object):
             log.error(result)
             raise
         if return_fitted_val:
-            return result.x, result.fun
+            return result.x, result.fun[0]
         return result.x

then causes test_inferapi_pyhf_independence to fail.

kratsg · 2020-07-21T23:46:28Z

qmu returns return tensorlib.clip(qmu, 0, max_value=None) which is likely converting it back into a tensor.

src/pyhf/optimize/opt_tflow.py

matthewfeickert · 2020-07-21T23:53:50Z

qmu returns return tensorlib.clip(qmu, 0, max_value=None) which is likely converting it back into a tensor.

No, if you give clip a scalar it just operates on that scalar (which I only learned today)

# On this branch
>>> import pyhf
>>> pyhf.set_backend("numpy")
>>> pyhf.tensorlib.clip(pyhf.tensorlib.astensor(1.), 0, max_value=None)
1.0
>>> pyhf.tensorlib.clip(1., 0, max_value=None)
1.0

matthewfeickert · 2020-07-22T03:36:33Z

The

        if return_fitted_val:
>           return result.x, result.fun[0]
E           TypeError: 'float' object is not subscriptable

that the CI is showing right now seems to be some form of Issue #964 as if I apply the following digg

diff --git a/tests/test_infer.py b/tests/test_infer.py
index 08953981..1cb16ae4 100644
--- a/tests/test_infer.py
+++ b/tests/test_infer.py
@@ -144,11 +144,12 @@ def test_inferapi_pyhf_independence():
             return tensorlib.astensor([main + constraint])
 
     model = NonPyhfModel([5, 50, 7])
-    cls = pyhf.infer.hypotest(
-        1.0, model.expected_data(model.config.suggested_init()), model
-    )
+    data = pyhf.tensorlib.astensor(model.expected_data(model.config.suggested_init()))
+    data = 1.05 * data
+    cls = pyhf.infer.hypotest(1.0, data, model)
+    print(cls)
 
-    assert np.isclose(cls[0], 0.7267836451638846)
+    # assert np.isclose(cls[0], 0.7267836451638846)

the pyhf.infer.hypotest(1.0, data, model) step is able to run without errors but all we've done is scale the data by 5%. This really doesn't seem to make any sense.

In Issue #964 @kratsg mentions that PR #951 should fix this sort of problem. I still need to review the PR, but can you also elaborate on how the PR achieves this?

matthewfeickert · 2020-07-22T05:44:03Z

I have things almost passing if I rebase this (in another branch) off of PR #951, so I'm going to put this back into draft mode and then we can come back to this after we've finished the other PR.

As many other manipulations eventually call torch.Tensor.view just skip steps and call it directly c.f. https://pytorch.org/docs/stable/tensors.html#torch.Tensor.view

github-actions · 2020-07-25T13:12:27Z

I've queued this up. When it gets merged, I'll create a minor release from v0.4.4 → v0.5.0 which includes the following 24 change(s) [including this PR]:

revert: Allow 0-dimensional shape tensors

feat: Add precision setting string API to set_backend

docs: Replace optimizer table with list

refactor: Optimizer API harmonization, minuit autodifferentiation support

fix: Enable jax.np.stack for axis !=0

chore: Set minimum version for iminuit to v1.4.3

docs: Add 2020 ATLAS Induction Day and PyHEP 2020 tutorials

fix: Protect against missing JAX imports

refactor: Use AsymptoticTestStatDistribution.expected_value API in hypotest for better integration

docs: Clarify role of return_fitted_val kwarg for fit and fixed_poi_fit

docs: Clarify objective function in infer.mle

feat: support no POI

docs: Add hepdata_like model API to docs

docs: Fix CLs wrapping and add shorter URL

feat: Enable shell completion

docs: Add examples of good contributions to CONTRIBUTING

docs: Add TRExFitter babel translation

docs: Unrestrict Sphinx from v3.0.X releases

chore: Future proof release against uproot4 API changes

fix: Change p-value calculation from 1-cdf(x) to cdf(-x)

revert: Rollback constraints on cloudpickle given TFP v0.10.1 release

fix: Set backend global states correctly

feat: Single precision setting for backends

ci: Run Docker test on tag pushes to avoid breaking CI

If you make any more changes, you probably want to re-trigger me again by removing the bumpversion/minor label and then adding it back again.

Triggered by #972 via GitHub Actions.

matthewfeickert added the revert Reverts a previous commit label Jul 21, 2020

matthewfeickert self-assigned this Jul 21, 2020

matthewfeickert commented Jul 21, 2020

View reviewed changes

src/pyhf/tensor/pytorch_backend.py Show resolved Hide resolved

matthewfeickert changed the title ~~revert: Allow empty shape tensors~~ revert: Allow 0-dimensional shape tensors Jul 21, 2020

matthewfeickert added the tests pytest label Jul 21, 2020

matthewfeickert commented Jul 21, 2020

View reviewed changes

src/pyhf/tensor/pytorch_backend.py Outdated Show resolved Hide resolved

matthewfeickert commented Jul 21, 2020

View reviewed changes

src/pyhf/tensor/pytorch_backend.py Outdated Show resolved Hide resolved

matthewfeickert added the docs Documentation related label Jul 21, 2020

matthewfeickert force-pushed the revert/allow-non-empty-shape branch from a89b2a7 to 0434d7a Compare July 21, 2020 09:55

matthewfeickert commented Jul 21, 2020

View reviewed changes

src/pyhf/infer/test_statistics.py Show resolved Hide resolved

matthewfeickert mentioned this pull request Jul 21, 2020

Investigate adding atleast_1d method to tensorlib #974

Open

matthewfeickert marked this pull request as ready for review July 21, 2020 16:58

matthewfeickert force-pushed the revert/allow-non-empty-shape branch from 0d2869a to c3b4f34 Compare July 21, 2020 16:59

matthewfeickert force-pushed the revert/allow-non-empty-shape branch from 952dbd0 to 1d6c9c7 Compare July 21, 2020 18:00

lukasheinrich reviewed Jul 21, 2020

View reviewed changes

src/pyhf/infer/test_statistics.py Outdated Show resolved Hide resolved

matthewfeickert requested review from kratsg and lukasheinrich July 21, 2020 23:26

kratsg reviewed Jul 21, 2020

View reviewed changes

src/pyhf/optimize/opt_tflow.py Outdated Show resolved Hide resolved

matthewfeickert mentioned this pull request Jul 22, 2020

bug: pyhf has unstable rerturn types to pyhf.infer.mle.fit #964

Closed

matthewfeickert marked this pull request as draft July 22, 2020 05:44

matthewfeickert added 19 commits July 25, 2020 09:08

Use torch.Tensor.view for faster op

07d4ae9

As many other manipulations eventually call torch.Tensor.view just skip steps and call it directly c.f. https://pytorch.org/docs/stable/tensors.html#torch.Tensor.view

Use Lukas's pattern as seems nicer

de66e09

q_mu should be a float

dfe5ddf

Ensure float is returned

d5ebe1f

Add atleast_1d method

a0f3fd0

Enforce scalar return for qmu

936a31e

Use list comprehensions

b964d89

Fix typo n docstrings

c3a03e2

Add clarifyng note

77cd4d1

Correct docs formatting

acd7117

Return list only if multiple tensors given

267d08e

Show that a single entry gives a tensor back

a1d5f72

Add stub of tests for test_atelast_1d

062c150

Restore harmonization in backends

16a0c85

Factor out adding atelast_1d test to another feat PR

de4ea16

Fix docstrings for mle

bad1cf1

Ensure returned objective function is a scalar

290466e

returned objective is still tensor, just 0-d so repr's as tensor

70435f5

Add mle.fit tests for shape of returns

385a6f8

kratsg force-pushed the revert/allow-non-empty-shape branch from 019f2c7 to 385a6f8 Compare July 25, 2020 13:09

kratsg added bumpversion/minor Create a minor version release and removed bumpversion/minor Create a minor version release labels Jul 25, 2020

scikit-hep deleted a comment from github-actions bot Jul 25, 2020

lukasheinrich approved these changes Jul 25, 2020

View reviewed changes

matthewfeickert merged commit 0ff43e8 into master Jul 25, 2020

matthewfeickert deleted the revert/allow-non-empty-shape branch July 25, 2020 16:50

kratsg pushed a commit that referenced this pull request Jul 25, 2020

Bump version: 0.4.4 → 0.5.0

27f35e9

Triggered by #972 via GitHub Actions.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: Allow 0-dimensional shape tensors #972

revert: Allow 0-dimensional shape tensors #972

matthewfeickert commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020

kratsg commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 22, 2020

matthewfeickert commented Jul 22, 2020 •

edited

Loading

github-actions bot commented Jul 25, 2020

revert: Allow 0-dimensional shape tensors #972

revert: Allow 0-dimensional shape tensors #972

Conversation

matthewfeickert commented Jul 21, 2020 • edited Loading

Description

Checklist Before Requesting Reviewer

Before Merging

matthewfeickert commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020 • edited Loading

matthewfeickert commented Jul 21, 2020

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

matthewfeickert commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020

kratsg commented Jul 21, 2020

matthewfeickert commented Jul 21, 2020 • edited Loading

matthewfeickert commented Jul 22, 2020

matthewfeickert commented Jul 22, 2020 • edited Loading

github-actions bot commented Jul 25, 2020

matthewfeickert commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 21, 2020 •

edited

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 21, 2020 •

edited

Loading

matthewfeickert commented Jul 22, 2020 •

edited

Loading