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

fix: Correct variable assignment for limit band artists #2411

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Dec 28, 2023

Description

We discovered a little bug in the brazil band visualization where the two sigma band was assigned to the "one_sigma_band" attribute of our artist collection and vice versa.

Fixing this now.

cc @malin-horstmann

Amends PR #1377

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
* Assign the one and two sigma band artists to the correct objects.
   - Amends PR https://github.com/scikit-hep/pyhf/pull/1377

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (970cd11) 98.28% compared to head (88bc5d5) 98.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2411   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files          69       69           
  Lines        4539     4539           
  Branches      803      803           
=======================================
  Hits         4461     4461           
  Misses         45       45           
  Partials       33       33           
Flag Coverage Δ
contrib 97.86% <100.00%> (ø)
doctest 60.71% <100.00%> (ø)
unittests-3.10 96.29% <0.00%> (ø)
unittests-3.11 96.29% <0.00%> (ø)
unittests-3.8 96.32% <0.00%> (ø)
unittests-3.9 96.34% <0.00%> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@malin-horstmann malin-horstmann left a comment

Choose a reason for hiding this comment

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

Looks great.

@lukasheinrich lukasheinrich changed the title Fix: This fixes the variable assignment for limit band artists [WIP] Fix: This fixes the variable assignment for limit band artists Dec 28, 2023
@lukasheinrich lukasheinrich changed the title Fix: This fixes the variable assignment for limit band artists fix: This fixes the variable assignment for limit band artists Dec 28, 2023
@matthewfeickert matthewfeickert added fix A bug fix contrib Targeting pyhf.contrib and not the core of pyhf visualization Related to plotting of results need-to-backport tmp label until can be backported to patch release branch labels Jan 4, 2024
@matthewfeickert matthewfeickert changed the title fix: This fixes the variable assignment for limit band artists fix: Correct variable assignment for limit band artists Jan 4, 2024
@matthewfeickert
Copy link
Member

Thanks @lukasheinrich and sorry for not noticing this until today (though with my laptop dead I would have had to review this on the phone anyway).

Here's an example that extends the docs example showing the bug

import numpy as np
import matplotlib.pyplot as plt
import pyhf
import pyhf.contrib.viz.brazil

pyhf.set_backend("numpy")
model  = pyhf.simplemodels.uncorrelated_background(
    signal=[12.0, 11.0], bkg=[50.0, 52.0], bkg_uncertainty=[3.0, 7.0]
)
observations = [51, 48]
data = observations + model.config.auxdata

test_pois = np.linspace(0, 5, 41)
results = [
    pyhf.infer.hypotest(test_poi, data, model, return_expected_set=True)
    for test_poi in test_pois
]
cls_obs = np.array([test[0] for test in results]).flatten()
cls_exp = [
    np.array([test[1][sigma_idx] for test in results]).flatten()
    for sigma_idx in range(5)
]
test_size = 0.05

fig, ax = plt.subplots()
artists = pyhf.contrib.viz.brazil.plot_brazil_band(
    test_pois, cls_obs, cls_exp, test_size, ax
)

one_sigma_band = artists[2]
one_sigma_band.set_color("blue")

ax.legend()

fig.savefig("pr_2411_bug.png")

pr_2411_bug

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thanks @lukasheinrich. I'll go ahead and finish the dependabot updates, then merge this, and get a patch release out with this fix in it.

@matthewfeickert matthewfeickert merged commit 4fdd6d6 into main Jan 4, 2024
23 checks passed
@matthewfeickert matthewfeickert deleted the fix_brazil_viz branch January 4, 2024 22:17
matthewfeickert added a commit that referenced this pull request Jan 5, 2024
)

* Backport PR #2411
* Assign the one and two sigma band artists to the correct objects.

Co-authored-by: Lukas <lukas.heinrich@gmail.com>
@matthewfeickert matthewfeickert removed the need-to-backport tmp label until can be backported to patch release branch label Jan 5, 2024
@matthewfeickert
Copy link
Member

This fix is now out in pyhf v0.7.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib Targeting pyhf.contrib and not the core of pyhf fix A bug fix visualization Related to plotting of results
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants