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

perf: cache yield_stdev using spec and interpcodes of model #322

Conversation

lhenkelm
Copy link
Contributor

@lhenkelm lhenkelm commented Feb 4, 2022

This implements the changes to hashing discussed in #315.

It was necessary to modify one test that was directly looking up cached results in the yield_stdev cache.

* caching for yield uncertainty calculation now based on model specification and interpolation codes
* caching now works when re-creating pyhf.pdf.Model objects

... instead of the model itself. In effect,
this makes the cache treat models like a value-typed hash.
In current pyhf (0.6.4 and below) Model inherits __hash__ from
object, so caching on model instances treats copies as different.
Copy link
Member

@alexander-held alexander-held left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing this! This is super useful to prevent accidental re-computation of computationally expensive results.

src/cabinetry/model_utils.py Outdated Show resolved Hide resolved
src/cabinetry/model_utils.py Outdated Show resolved Hide resolved
src/cabinetry/model_utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #322 (766573b) into master (40120c2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #322   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         1881      1889    +8     
  Branches       305       306    +1     
=========================================
+ Hits          1881      1889    +8     
Impacted Files Coverage Δ
src/cabinetry/model_utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40120c2...766573b. Read the comment docs.

@alexander-held
Copy link
Member

Thank you very much for the PR, this all looks good to me. I'd like to see whether we can get a resolution of scikit-hep/pyhf#1762 (comment), but as far as I can tell this has no impact for the equality comparison done here (where we care about equal expected_data output). Maybe worth adding another sentence to the docstring to prevent possible future confusion: the key ignores measurement config information, but as far as I can tell that matters for neither logpdf nor expected_data, and only for configuration information like config.suggested_bounds().

@lhenkelm
Copy link
Contributor Author

lhenkelm commented Feb 7, 2022

Thanks for reviewing! I added the warning to the docstring.
In the specific case of yield_stdev I agree this makes no difference, since no fits will be done and a full fit result is passed in alongside the model. In general the best thing would be to have pyhf objects that are deliberate about how they hash (in so far as they model values), since capturing everything would involve diving deep into the tree of pyhf objects, many of them internal to pyhf.

Copy link
Member

@alexander-held alexander-held 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, thanks again!

@alexander-held alexander-held merged commit f34c73a into scikit-hep:master Feb 8, 2022
@lhenkelm lhenkelm deleted the perf/yield-stdev-cache-model-value-like branch February 8, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants