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

feat: par_names handles non-scalar modifiers with 1 parameter #1560

Merged
merged 3 commits into from
Aug 25, 2021

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 25, 2021

Pull Request Description

Resolves #867.

Should fully resolve the last remaining parts of #867. There are two issues handled:

  • par_names did allow for customizable fstring, but this would never get passed through to the underlying optimizers that use it, so it's being dropped as the benefit of configurability is outweighed by the extra work on the user to maintain and bookkeep
  • par_names now correctly uses the fact that parameter sets which are non-scalar but only have one parameter (e.g. channel of 1 bin) will still be named with the index {parameter}[0] rather than just {parameter} to clarify that it's a non-scalar parameter explicitly (but also somewhat implicitly).

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
* change par_names API to drop fstring configurability
* modify par_names to handle non-scalar parameters in 1-bin channels

@kratsg kratsg added API Changes the public API feat/enhancement New feature or request fix A bug fix follow up labels Aug 25, 2021
@kratsg kratsg self-assigned this Aug 25, 2021
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #1560 (c203b14) into master (bd1d571) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1560   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          63       63           
  Lines        4050     4050           
  Branches      576      576           
=======================================
  Hits         3957     3957           
  Misses         54       54           
  Partials       39       39           
Flag Coverage Δ
contrib 25.40% <33.33%> (ø)
unittests 97.48% <100.00%> (ø)

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

Impacted Files Coverage Δ
src/pyhf/pdf.py 96.54% <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 bd1d571...c203b14. Read the comment docs.

@kratsg
Copy link
Contributor Author

kratsg commented Aug 25, 2021

As the changes are pretty minor (especially given the original functionality was ok in the originating PR), I'm going to merge this in so @matthewfeickert can focus on getting the release out.

@kratsg kratsg merged commit e2b62e9 into master Aug 25, 2021
@kratsg kratsg deleted the feat/improvedParNameHandling branch August 25, 2021 00:41
@matthewfeickert
Copy link
Member

@kratsg as this is API breaking do we know if this is problematic for cabinetry at all? cc @alexander-held.

I'm optimistic that is isn't a problem, given that you only had to add a test and not remove any.

@kratsg
Copy link
Contributor Author

kratsg commented Aug 25, 2021

not API breaking as this wasn't in the v0.6.2 API. It's just changes to the API on the main branch.

@matthewfeickert
Copy link
Member

Ah that's fantastic. Thanks for clarifying that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API feat/enhancement New feature or request fix A bug fix follow up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obtaining fit parameter labels
2 participants