Skip to content

chore: [pre-commit.ci] pre-commit autoupdate#2264

Merged
matthewfeickert merged 7 commits intomainfrom
pre-commit-ci-update-config
Aug 10, 2023
Merged

chore: [pre-commit.ci] pre-commit autoupdate#2264
matthewfeickert merged 7 commits intomainfrom
pre-commit-ci-update-config

Conversation

@pre-commit-ci
Copy link
Contributor

@pre-commit-ci pre-commit-ci bot commented Aug 1, 2023 •
edited by matthewfeickert
Loading

updates:

* Update pre-commit hooks:
   - github.com/astral-sh/ruff-pre-commit: v0.0.276 → v0.0.281
   - github.com/psf/black-pre-commit-mirror: 23.3.0 → 23.7.0
   - github.com/asottile/blacken-docs: 1.14.0 → 1.15.0
* Apply Ruff's fix of using `next` on a iterator over building a
  list and then getting the 0th item.

@matthewfeickert matthewfeickert self-assigned this Aug 1, 2023
@matthewfeickert matthewfeickert added chore Other changes that don't modify src or test files need-to-backport tmp label until can be backported to patch release branch labels Aug 1, 2023
@matthewfeickert matthewfeickert self-requested a review August 1, 2023 04:42
@codecov
Copy link

codecov bot commented Aug 1, 2023 •
edited
Loading

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.03% ⚠️

Comparison is base (2204e99) 98.30% compared to head (2ec8ee6) 98.27%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2264      +/-   ##
==========================================
- Coverage   98.30%   98.27%   -0.03%     
==========================================
  Files          69       69              
  Lines        4534     4534              
  Branches      802      801       -1     
==========================================
- Hits         4457     4456       -1     
  Misses         45       45              
- Partials       32       33       +1
Flag Coverage Δ
contrib 97.86% <50.00%> (-0.03%) ⬇️
doctest 61.07% <0.00%> (-0.03%) ⬇️
unittests-3.10 96.31% <0.00%> (ø)
unittests-3.11 96.31% <0.00%> (ø)
unittests-3.8 96.33% <0.00%> (ø)
unittests-3.9 96.36% <0.00%> (ø)

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

Files Changed Coverage Δ
src/pyhf/contrib/viz/brazil.py 98.46% <0.00%> (-1.54%) ⬇️
src/pyhf/contrib/utils.py 100.00% <100.00%> (ø)

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

@matthewfeickert matthewfeickert added the pre-commit Related to pre-commit hooks label Aug 10, 2023
pre-commit-ci bot and others added 3 commits August 10, 2023 02:01
@pre-commit-ci @matthewfeickert
chore: [pre-commit.ci] pre-commit autoupdate
363350c 
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.0.276 → v0.0.281](astral-sh/ruff-pre-commit@v0.0.276...v0.0.281)
- [github.com/psf/black: 23.3.0 → 23.7.0](psf/black@23.3.0...23.7.0)
- [github.com/asottile/blacken-docs: 1.14.0 → 1.15.0](adamchainz/blacken-docs@1.14.0...1.15.0)
@pre-commit-ci @matthewfeickert
[pre-commit.ci] auto fixes from pre-commit.com hooks
6790ace 
for more information, see https://pre-commit.ci
@matthewfeickert
Apply additional_dependencies
b8faaa6
@matthewfeickert matthewfeickert force-pushed the pre-commit-ci-update-config branch from a2d44b4 to b8faaa6 Compare August 10, 2023 07:06
matthewfeickert
matthewfeickert reviewed Aug 10, 2023
View reviewed changes
src/pyhf/contrib/viz/brazil.py
Copy link
Member

@matthewfeickert matthewfeickert Aug 10, 2023

Choose a reason for hiding this comment

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

It is unclear to me why coverage is reporting the [0] case as being fully covered but the next case as only partial coverage. :?

Copy link
Member

@matthewfeickert matthewfeickert Aug 10, 2023

Choose a reason for hiding this comment

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

Ah. Seems because help(next) shows

Help on built-in function next in module builtins:

next(...)
    next(iterator[, default])
    
    Return the next item from the iterator. If default is given and the iterator
    is exhausted, it is returned instead of raising StopIteration.

that coveragepy/coveragepy#605 (comment) and coveragepy/coveragepy#1617 might be relevant here.

Copy link
Member

@matthewfeickert matthewfeickert Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@kratsg Is there a way to mock or do something else to get

pyhf/tests/contrib/test_viz.py

Lines 16 to 23 in 6c35247

def test_brazil_band_collection(datadir):
data = json.load(datadir.joinpath("hypotest_results.json").open(encoding="utf-8"))
fig = Figure()
ax = fig.subplots()
brazil_band_collection = brazil.plot_results(
data["testmus"], data["results"], test_size=0.05, ax=ax
)

to cause

pyhf/src/pyhf/contrib/viz/brazil.py

Lines 360 to 366 in 6c35247

# Order legend: ensure CLs expected band and test size are last in legend
handles, labels = ax.get_legend_handles_labels()
if not no_cls:
for label_part in ["exp", "pm1", "pm2", "alpha"]:
label_idx = [
idx for idx, label in enumerate(labels) if label_part in label
][0]

to raise a StopIteration when switched to using next? So to mock the value of labels such that one of the label_part values aren't found?

By construction the function should always get the values, so this might be an area where the dip in the test coverage by partial for 1 line is worth it.

Copy link
Member

@matthewfeickert matthewfeickert Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, this can just be mocked and so can just be a follow up PR.

@matthewfeickert matthewfeickert added the tests pytest label Aug 10, 2023
@matthewfeickert matthewfeickert merged commit 6f8ad2e into main Aug 10, 2023
@matthewfeickert matthewfeickert deleted the pre-commit-ci-update-config branch August 10, 2023 16:08
@matthewfeickert matthewfeickert mentioned this pull request Aug 10, 2023
Merged
4 tasks
kratsg pushed a commit that referenced this pull request Aug 10, 2023
@matthewfeickert
test: Add test for iterator exhaustion with next (#2273)
852a224 
* Add test to tests/contrib/test_viz.py to cover the case where the
  wrong labels are somehow returned in brazil.plot_results, causing the
  legend label ordering logic to fail.
* Amends PR #2264
@matthewfeickert matthewfeickert mentioned this pull request Aug 16, 2023
Merged
4 tasks
@matthewfeickert matthewfeickert removed the need-to-backport tmp label until can be backported to patch release branch label Aug 16, 2023
matthewfeickert added a commit that referenced this pull request Aug 16, 2023
@matthewfeickert
chore(backport): Update pre-commit hooks (#2283)
3639975 
* Backport components of:
   - #2227
   - #2239
   - #2240
   - #2264
   - #2270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewfeickert matthewfeickert matthewfeickert left review comments

Assignees

@matthewfeickert matthewfeickert

Labels

chore Other changes that don't modify src or test files pre-commit Related to pre-commit hooks tests pytest

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@matthewfeickert