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.

Sign up for GitHub

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

Jump to bottom

fix: Use function scope to avoid altering hypotest_args fixture #2011

Merged
merged 3 commits into from
Sep 20, 2022
Merged

fix: Use function scope to avoid altering hypotest_args fixture #2011

merged 3 commits into from
Sep 20, 2022

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Sep 20, 2022 •
edited
Loading

Description

Needed for PR #1274

  • Use the 'function' scope for hypotest_args fixture. Fixing the POI in the model config changes it for the entire fixture if the fixture is 'module' scope, which would cause errors in other tests that later use it and defeats the point of the fixture.
  • Remove unnecessary use of tmpdir fixture as no output is written.

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
* Use the 'function' scope for hypotest_args fixture. Fixing the POI in the model config
  changes it for the entire fixture if the fixture is 'module' scope, which would cause
  errors in other tests that later use it and defeats the point of the fixture.
* Remove unnecessary use of tmpdir fixture as no output is written.

@matthewfeickert matthewfeickert added tests pytest fix A bug fix labels Sep 20, 2022
@matthewfeickert matthewfeickert self-assigned this Sep 20, 2022
@matthewfeickert matthewfeickert mentioned this pull request Sep 20, 2022
Merged
4 tasks
@codecov
Copy link

codecov bot commented Sep 20, 2022 •
edited
Loading

Codecov Report

Base: 98.28% // Head: 97.99% // Decreases project coverage by -0.29% ⚠️

Coverage data is based on head (df1fb4d) compared to base (2a326f7).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2011      +/-   ##
==========================================
- Coverage   98.28%   97.99%   -0.30%     
==========================================
  Files          68       68              
  Lines        4482     4482              
  Branches      730      730              
==========================================
- Hits         4405     4392      -13     
- Misses         45       55      +10     
- Partials       32       35       +3
Flag Coverage Δ
contrib 27.64% <ø> (ø)
doctest ?
unittests-3.10 96.22% <ø> (ø)
unittests-3.7 96.20% <ø> (ø)
unittests-3.8 96.25% <ø> (ø)
unittests-3.9 96.27% <ø> (ø)

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

Impacted Files Coverage Δ
src/pyhf/infer/intervals.py 90.00% <0.00%> (-10.00%) ⬇️
src/pyhf/tensor/pytorch_backend.py 94.69% <0.00%> (-3.79%) ⬇️
src/pyhf/infer/test_statistics.py 96.66% <0.00%> (-3.34%) ⬇️
src/pyhf/tensor/tensorflow_backend.py 95.33% <0.00%> (-2.00%) ⬇️
src/pyhf/tensor/jax_backend.py 97.90% <0.00%> (-0.70%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kratsg
kratsg requested changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

Just change the fixture scope to function instead and you don't need the deepcopy. This should remove the need for deepcopy.

tests/test_infer.py Outdated Show resolved Hide resolved
@matthewfeickert
fix: Use deepcopy to avoid altering hypotest_args fixture
fad7bdb 
* Use copy.deepcopy to create a copy of the pyhf.simplemodels.uncorrelated_background
  object from the hypotest_args fixture. Fixing the POI in the model
  config changes it for the entire fixture otherwise, which would cause
  errors in other tests that later use it and defeats the point of the
  fixture.
* Remove unnecessary use of tmpdir fixture as no output is written.
@matthewfeickert
Use function scope for fixture
b3418fb
@matthewfeickert
Unpack for clarity
df1fb4d
@matthewfeickert matthewfeickert force-pushed the fix/avoid-fixture-altering branch from 6cdcc93 to df1fb4d Compare September 20, 2022 14:16
@matthewfeickert matthewfeickert changed the title fix: Use deepcopy to avoid altering hypotest_args fixture fix: Use function scope to avoid altering hypotest_args fixture Sep 20, 2022
@kratsg
Copy link
Contributor

kratsg commented Sep 20, 2022

The coverage failure/change here is rather suspicious / odd as none of the changes should impact coverage.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Sep 20, 2022 •
edited
Loading

The coverage failure/change here is rather suspicious / odd as none of the changes should impact coverage.

Yeah I think Codecov is having issues. :/

@matthewfeickert matthewfeickert merged commit f7e974f into master Sep 20, 2022
@matthewfeickert matthewfeickert deleted the fix/avoid-fixture-altering branch September 20, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kratsg kratsg kratsg approved these changes

@lukasheinrich lukasheinrich Awaiting requested review from lukasheinrich

Assignees

@matthewfeickert matthewfeickert

Labels
fix A bug fix tests pytest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewfeickert @kratsg