Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: change CLs to be scalar #714

Closed
lukasheinrich opened this issue Dec 30, 2019 · 5 comments · Fixed by #944
Closed

API: change CLs to be scalar #714

lukasheinrich opened this issue Dec 30, 2019 · 5 comments · Fixed by #944
Labels
API Changes the public API good first issue Good for newcomers

Comments

@lukasheinrich
Copy link
Contributor

Description

right now it's returning (1,) vectors

@matthewfeickert matthewfeickert added API Changes the public API good first issue Good for newcomers labels Jul 9, 2020
@matthewfeickert
Copy link
Member

matthewfeickert commented Jul 11, 2020

Quick followup:

>>> import pyhf
>>> pyhf.set_backend("numpy")
>>> model = pyhf.simplemodels.hepdata_like(
...    signal_data=[12.0, 11.0], bkg_data=[50.0, 52.0], bkg_uncerts=[3.0, 7.0]
>>> )
>>> observations = [51, 48]
>>> data = observations + model.config.auxdata
>>> test_poi = 1.0
>>> CLs_obs, CLs_exp_band = pyhf.infer.hypotest(
...    test_poi, data, model, qtilde=True, return_expected_set=True
>>> )
>>> print(CLs_obs)
[0.05251554]
>>> type(CLs_obs)
<class 'numpy.ndarray'>

but the CLs is a scalar by definition, so there is no need to return it in a Tensor (as Lukas is pointing out above).

@kratsg
Copy link
Contributor

kratsg commented Jul 11, 2020

We might need to consider the implications -- e.g. -- for autodiff or machine learning libs that need to use feedback?

@matthewfeickert
Copy link
Member

matthewfeickert commented Jul 12, 2020

We might need to consider the implications -- e.g. -- for autodiff or machine learning libs that need to use feedback?

Maybe I'm not thinking about it clearly, but as the CLs is the end result and a scalar it won't have a gradient associated to it. @lukasheinrich @phinate can you clarify for me (my brain is currently very foggy)?

@lukasheinrich
Copy link
Contributor Author

forr backprop in particular it needs to be a scalar (we just index into it right now). The reason we had it be a (1,) vector is because (i think) pytorch didn't support pure scalars tensors. But now they all do

In [4]: torch.tensor(1.0).shape                                                                                                                                                                                 
Out[4]: torch.Size([])

In [6]: tf.constant(1.0)                                                                                                                                                                                        
2020-07-12 09:35:46.134964: I tensorflow/core/platform/cpu_feature_guard.cc:143] Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 FMA
2020-07-12 09:35:46.155129: I tensorflow/compiler/xla/service/service.cc:168] XLA service 0x7fbacd8d04c0 initialized for platform Host (this does not guarantee that XLA will be used). Devices:
2020-07-12 09:35:46.155149: I tensorflow/compiler/xla/service/service.cc:176]   StreamExecutor device (0): Host, Default Version
Out[6]: <tf.Tensor: shape=(), dtype=float32, numpy=1.0>


In [9]: jax.numpy.array(0.0)                                                                                                                                                                                    
/Users/lukasheinrich/Code/pyhfdev/dev/pyhfdevenv/lib/python3.7/site-packages/jax/lib/xla_bridge.py:125: UserWarning: No GPU/TPU found, falling back to CPU.
  warnings.warn('No GPU/TPU found, falling back to CPU.')
Out[9]: DeviceArray(0., dtype=float32)

@matthewfeickert
Copy link
Member

matthewfeickert commented Jul 14, 2020

This is an easy change, but this will change all the docs and notebooks. This is prehaps minor enough that people might not care, but as I think people are mainly looking at the hypotest docs should this be held off from going into master (even if the PR is ready) until v0.5.0 is just about to go in?

Or are you happy with just returning a scalar ndarray, and not a float?

# numpy
>>> import numpy as np
>>> x = np.array([0.5])
>>> y = np.reshape(x, ())
>>> z = x[0]
>>> print(f"y is a {type(y)} of shape {y.shape} and has value {y}")
y is a <class 'numpy.ndarray'> of shape () and has value 0.5
>>> print(f"z is a {type(z)} of shape {z.shape} and has value {z}")
z is a <class 'numpy.float64'> of shape () and has value 0.5
# torch
>>> import torch
>>> x = torch.tensor([0.5])
>>> y = torch.reshape(x, ())
>>> z = x[0]
>>> print(f"y is a {type(y)} of shape {y.shape} and has value {y}")
y is a <class 'torch.Tensor'> of shape torch.Size([]) and has value 0.5
>>> print(f"z is a {type(z)} of shape {z.shape} and has value {z}")
z is a <class 'torch.Tensor'> of shape torch.Size([]) and has value 0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants