Skip to content

fix: Cast non-hashable types to floats in toms748_scan #2467

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/pyhf/infer/intervals/upper_limits.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ def __dir__():

def _interp(x, xp, fp):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _interp(x, xp, fp):
def _interp(x, xp: list[float], fp):

tb, _ = get_backend()
return tb.astensor(np.interp(x, xp.tolist(), fp.tolist()))
# xp has already been turned into a list at this point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can drop this comment with a typehint above as suggested

return tb.astensor(np.interp(x, xp, fp.tolist()))


def toms748_scan(
Expand Down Expand Up @@ -79,19 +80,29 @@ def toms748_scan(

def f_cached(poi):
if poi not in cache:
cache[poi] = hypotest(
# FIXME: scipy.optimize.toms748 still operates on floats,
# not any form of ndarray, so want everything in the
# cache to be a float.
# This may change with the Python array API standard
# in the future.
cls_obs, cls_exp_band = hypotest(
poi,
data,
model,
return_expected_set=True,
**hypotest_kwargs,
)
cache[poi] = (float(cls_obs), [float(x) for x in cls_exp_band])
return cache[poi]

def f(poi, level, limit=0):
# Use integers for limit so we don't need a string comparison
# limit == 0: Observed
# else: expected

# Arrays are not hashable types, so cast to float
poi = float(poi)

return (
f_cached(poi)[0] - level
if limit == 0
Expand Down Expand Up @@ -194,7 +205,9 @@ def linear_grid_scan(
obs = tb.astensor([[r[0]] for r in results])
exp = tb.astensor([[r[1][idx] for idx in range(5)] for r in results])

result_array = tb.concatenate([obs, exp], axis=1).T
# TODO: Can use `.T` after TensorFlow support is removed.
result_array = tb.transpose(tb.concatenate([obs, exp], axis=1))
result_array = tb.tolist(result_array)

# observed limit and the (0, +-1, +-2)sigma expected limits
limits = [_interp(level, result_array[idx][::-1], scan[::-1]) for idx in range(6)]
Expand Down
64 changes: 40 additions & 24 deletions tests/test_infer.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def check_uniform_type(in_list):
)


def test_toms748_scan(tmp_path, hypotest_args):
def test_toms748_scan(backend, tmp_path, hypotest_args):
"""
Test the upper limit toms748 scan returns the correct structure and values
"""
Expand Down Expand Up @@ -53,11 +53,12 @@ def test_toms748_scan(tmp_path, hypotest_args):
for i in range(5)
]
)
assert observed_cls == pytest.approx(0.05)
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_cls) == pytest.approx(0.05)
assert expected_cls == pytest.approx(0.05)


def test_toms748_scan_bounds_extension(hypotest_args):
def test_toms748_scan_bounds_extension(backend, hypotest_args):
"""
Test the upper limit toms748 scan bounds can correctly extend to bracket the CLs level
"""
Expand All @@ -72,18 +73,20 @@ def test_toms748_scan_bounds_extension(hypotest_args):
data, model, 3, 5, rtol=1e-8
)

assert observed_limit == pytest.approx(observed_limit_default)
# FIXME: Remove float cast after TensorFlow support removed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kratsg Running the TensorFlow tests is very slow and adds minutes to this run. As we're going to be dropping TensorFlow in the future, I think that we could probably just skip TensorFlow in these tests. Thoughts?

assert float(observed_limit) == pytest.approx(observed_limit_default)
assert np.allclose(np.asarray(expected_limits), np.asarray(expected_limits_default))

# Force bounds_up to expand
observed_limit, expected_limits = pyhf.infer.intervals.upper_limits.toms748_scan(
data, model, 0, 1, rtol=1e-8
)
assert observed_limit == pytest.approx(observed_limit_default)
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(observed_limit_default)
assert np.allclose(np.asarray(expected_limits), np.asarray(expected_limits_default))


def test_upper_limit_against_auto(hypotest_args):
def test_upper_limit_against_auto(backend, hypotest_args):
"""
Test upper_limit linear scan and toms748_scan return similar results
"""
Expand All @@ -97,11 +100,13 @@ def test_upper_limit_against_auto(hypotest_args):
)
obs_linear, exp_linear = results_linear
# Can't expect these to be much closer given the low granularity of the linear scan
assert obs_auto == pytest.approx(obs_linear, abs=0.1)
# FIXME: Remove float cast after TensorFlow support removed
assert float(obs_auto) == pytest.approx(obs_linear, abs=0.1)
assert np.allclose(exp_auto, exp_linear, atol=0.1)


def test_upper_limit(hypotest_args):
@pytest.mark.skip_numpy_minuit
def test_upper_limit(backend, hypotest_args):
"""
Check that the default return structure of pyhf.infer.hypotest is as expected
"""
Expand All @@ -110,22 +115,27 @@ def test_upper_limit(hypotest_args):
results = pyhf.infer.intervals.upper_limits.upper_limit(data, model, scan=scan)
assert len(results) == 2
observed_limit, expected_limits = results
assert observed_limit == pytest.approx(1.0262704738584554)
assert expected_limits == pytest.approx(
[0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(1.0262704738584554)
# FIXME: Can use expected_limits == pytest.approx([...]) after TensorFlow support removed
assert np.allclose(
expected_limits, [0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
)

# tighter relative tolerance needed for macos
results = pyhf.infer.intervals.upper_limits.upper_limit(data, model, rtol=1e-6)
assert len(results) == 2
observed_limit, expected_limits = results
assert observed_limit == pytest.approx(1.01156939)
assert expected_limits == pytest.approx(
[0.55988001, 0.75702336, 1.06234693, 1.50116923, 2.05078596]
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(1.01156939)
# FIXME: Can use expected_limits == pytest.approx([...]) after TensorFlow support removed
assert np.allclose(
expected_limits, [0.55988001, 0.75702336, 1.06234693, 1.50116923, 2.05078596]
)


def test_upper_limit_with_kwargs(hypotest_args):
@pytest.mark.skip_numpy_minuit
def test_upper_limit_with_kwargs(backend, hypotest_args):
"""
Check that the default return structure of pyhf.infer.hypotest is as expected
"""
Expand All @@ -136,9 +146,11 @@ def test_upper_limit_with_kwargs(hypotest_args):
)
assert len(results) == 2
observed_limit, expected_limits = results
assert observed_limit == pytest.approx(1.0262704738584554)
assert expected_limits == pytest.approx(
[0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(1.0262704738584554)
# FIXME: Can use expected_limits == pytest.approx([...]) after TensorFlow support removed
assert np.allclose(
expected_limits, [0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
)

# linear_grid_scan
Expand All @@ -147,9 +159,11 @@ def test_upper_limit_with_kwargs(hypotest_args):
)
assert len(results) == 3
observed_limit, expected_limits, (_scan, point_results) = results
assert observed_limit == pytest.approx(1.0262704738584554)
assert expected_limits == pytest.approx(
[0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(1.0262704738584554)
# FIXME: Can use expected_limits == pytest.approx([...]) after TensorFlow support removed
assert np.allclose(
expected_limits, [0.65765653, 0.87999725, 1.12453992, 1.50243428, 2.09232927]
)
assert _scan.tolist() == scan.tolist()
assert len(_scan) == len(point_results)
Expand All @@ -160,9 +174,11 @@ def test_upper_limit_with_kwargs(hypotest_args):
)
assert len(results) == 3
observed_limit, expected_limits, (_scan, point_results) = results
assert observed_limit == pytest.approx(1.01156939)
assert expected_limits == pytest.approx(
[0.55988001, 0.75702336, 1.06234693, 1.50116923, 2.05078596]
# FIXME: Remove float cast after TensorFlow support removed
assert float(observed_limit) == pytest.approx(1.01156939)
# FIXME: Can use expected_limits == pytest.approx([...]) after TensorFlow support removed
assert np.allclose(
expected_limits, [0.55988001, 0.75702336, 1.06234693, 1.50116923, 2.05078596]
)


Expand Down