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

docs: Document Channel Summary Mixin #1972

Merged
merged 9 commits into from Sep 4, 2022

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Aug 30, 2022

Pull Request Description

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
* Make _ChannelSummaryMixin parameter related attributes properties.
* Add documentation for parameter related properties of _ModelConfig by documenting
  the _ChannelSummaryMixin.
* Add typehints to mixins.
* Update tests to mock the properties.

@kratsg kratsg added the docs Documentation related label Aug 30, 2022
@kratsg kratsg self-assigned this Aug 30, 2022
@kratsg kratsg added this to In progress in v0.7.0 via automation Aug 30, 2022
@kratsg
Copy link
Contributor Author

kratsg commented Aug 30, 2022

Screen Shot 2022-08-30 at 12 09 49 PM

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Base: 98.24% // Head: 98.25% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (18b3cde) compared to base (b6e02ee).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1972      +/-   ##
==========================================
+ Coverage   98.24%   98.25%   +0.01%     
==========================================
  Files          68       68              
  Lines        4378     4416      +38     
  Branches      726      726              
==========================================
+ Hits         4301     4339      +38     
  Misses         45       45              
  Partials       32       32              
Flag Coverage Δ
contrib 26.97% <42.42%> (+0.38%) ⬆️
doctest 60.89% <98.48%> (+0.31%) ⬆️
unittests-3.10 96.17% <100.00%> (+0.03%) ⬆️
unittests-3.7 96.14% <100.00%> (+0.02%) ⬆️
unittests-3.8 96.19% <100.00%> (+0.03%) ⬆️
unittests-3.9 96.21% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/mixins.py 100.00% <100.00%> (ø)
src/pyhf/pdf.py 97.98% <100.00%> (+0.12%) ⬆️

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 closed this Aug 31, 2022
v0.7.0 automation moved this from In progress to Done Aug 31, 2022
@kratsg kratsg reopened this Aug 31, 2022
v0.7.0 automation moved this from Done to In progress Aug 31, 2022
@matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert added the type checking Related to types and type checking label Sep 3, 2022
tests/test_workspace.py Outdated Show resolved Hide resolved
v0.7.0 automation moved this from In progress to Review in progress Sep 3, 2022
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.

I might have misunderstood the purpose of the changes, but I do not see model.config.parameters / .par_order mentioned in #1747 in https://pyhf--1972.org.readthedocs.build/en/1972/_generated/pyhf.pdf._ModelConfig.html. .par_names() had already been included previously, and that is very useful, but I think .par_order would also be needed. I think .parameters is not particularly relevant and can be obtained by sorting .par_order instead.

I would also suggest somehow pointing to _ModelConfig from the pyhf.pdf.Model page. The .config API is not listed there, and it is not particularly intuitive for a user to look at _ModelConfig to find out about that .config attribute, where it says

_ModelConfig should not be called directly. It should instead by accessed through the config attribute of Model

Given that _ModelConfig implies internal API, I wonder whether all of this page should just live within the Model API documentation page? This is where I would expect a user to look for it.

@kratsg
Copy link
Contributor Author

kratsg commented Sep 3, 2022

I might have misunderstood the purpose of the changes, but I do not see model.config.parameters / .par_order mentioned in #1747 in https://pyhf--1972.org.readthedocs.build/en/1972/_generated/pyhf.pdf._ModelConfig.html. .par_names() had already been included previously, and that is very useful, but I think .par_order would also be needed. I think .parameters is not particularly relevant and can be obtained by sorting .par_order instead.

par_order is on _ModelConfig and not the mixin. This PR is just documenting the mixin.

I would also suggest somehow pointing to _ModelConfig from the pyhf.pdf.Model page. The .config API is not listed there, and it is not particularly intuitive for a user to look at _ModelConfig to find out about that .config attribute, where it says

I'll add this in this PR.

The fundamental reason that par_order and others didn't show up is because they were object attributes and not class attributes (so sphinx won't see it by introspection).

@alexander-held
Copy link
Member

Looks like I was a bit too slow and created #1982 before the edit of the previous comment, so that would be another issue this PR closes.

I see that par_order is also there now, plus the other _ModelConfig pieces so this looks great. Thank you!

@alexander-held
Copy link
Member

I confused myself and was looking at _ModelConfig instead of Model where par_order etc. appear, but given that this is now cross-linked through the .config property of Model this is discoverable and I think that works well.

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.

LGTM. 👍 Thanks @kratsg — this will help!

@matthewfeickert matthewfeickert added the tests pytest label Sep 4, 2022
@matthewfeickert matthewfeickert merged commit 0fe3434 into master Sep 4, 2022
@matthewfeickert matthewfeickert deleted the docs/documentChannelSummaryMixin branch September 4, 2022 19:49
v0.7.0 automation moved this from Review in progress to Done Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related tests pytest type checking Related to types and type checking
Projects
3 participants