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

Add support for inhomogenous error breakdowns #265

Merged
merged 7 commits into from
Jul 25, 2024

Conversation

20DM
Copy link
Contributor

@20DM 20DM commented Jul 22, 2024

Proposal solution for the feature discussed in #229 :

The HepData validator already allows for cases where different bins have different error breakdowns, but hepdata_lib currently does not support this.

With this change set one can suppress individual sources by setting them to None.

Closes #266.


📚 Documentation preview 📚: https://hepdata-lib--265.org.readthedocs.build/en/265/

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (dcc23bd) to head (2b8c075).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #265      +/-   ##
==========================================
+ Coverage   90.37%   90.42%   +0.04%     
==========================================
  Files           5        5              
  Lines        1112     1117       +5     
  Branches      251      254       +3     
==========================================
+ Hits         1005     1010       +5     
  Misses         78       78              
  Partials       29       29              
Flag Coverage Δ
unittests-3.10 90.42% <100.00%> (+0.04%) ⬆️
unittests-3.11 90.42% <100.00%> (+0.04%) ⬆️
unittests-3.6 90.14% <100.00%> (+0.04%) ⬆️
unittests-3.7 90.14% <100.00%> (+0.04%) ⬆️
unittests-3.8 90.24% <100.00%> (+0.04%) ⬆️
unittests-3.9 90.24% <100.00%> (+0.04%) ⬆️

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

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

Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you please add a test in tests/test_uncertainty.py to cover the new feature and add some explanation under Uncertainties in the documentation?

@GraemeWatt
Copy link
Member

Thanks for adding the test and expanding the docs. The CI is failing due to issues with pylint. Can you please modify the code so that the pylint checks pass? See Analysing the code in the developer docs.

Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the linting for the tests. However, the CI still gives a message:

************* Module hepdata_lib
hepdata_lib/__init__.py:237:4: R0912: Too many branches (13/12) (too-many-branches)

-----------------------------------
Your code has been rated at 9.99/10

Could you please fix this too? If you don't see an easy way to refactor the make_dict function to avoid this warning, it can be suppressed by adding a line # pylint: disable=too-many-branches at the top of the function, but this should be avoided if possible. I think it's a current bug in the CI that the "Run pylint" still passes, so I've opened a separate PR #267 to address it.

hepdata_lib/helpers.py Show resolved Hide resolved
@GraemeWatt
Copy link
Member

PR #267 has been merged now, so if you update your branch with the last commit to main (dcc23bd), the CI should now fail at the "Run pylint on hepdata_lib" step.

@20DM
Copy link
Contributor Author

20DM commented Jul 23, 2024

Thanks for fixing the linting for the tests. However, the CI still gives a message:

************* Module hepdata_lib
hepdata_lib/__init__.py:237:4: R0912: Too many branches (13/12) (too-many-branches)

-----------------------------------
Your code has been rated at 9.99/10

Could you please fix this too? If you don't see an easy way to refactor the make_dict function to avoid this warning, it can be suppressed by adding a line # pylint: disable=too-many-branches at the top of the function, but this should be avoided if possible. I think it's a current bug in the CI that the "Run pylint" still passes, so I've opened a separate PR #267 to address it.

Urgh - well, this can be circumvented by moving the additional if statement into a separate list comprehensions, e.g. like so

for unc in [ unc for unc in self.uncertainties if not unc.values[i] is None ]:
    ...

but that adds an extra loop for no good reason. So my preference would be to silence the linter here, but I'd leave that call to you. 🤷

Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

I've added # pylint: disable=too-many-branches at the top of the make_dict function, added NoneType in the docstring of the sanitize_value function, and merged the last commit to the main branch. The workflow run for the separate "Run pylint on hepdata_lib" step now fails before adding the # pylint: disable=too-many-branches line, as expected. I'll let @clelange perform the final review and merge.

@GraemeWatt GraemeWatt requested a review from clelange July 23, 2024 15:13
Copy link
Collaborator

@clelange clelange left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @20DM and for the review @GraemeWatt !

@clelange clelange merged commit 11bc398 into HEPData:main Jul 25, 2024
25 checks passed
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.

Support inhomogeneous uncertainties
3 participants