HEPData / hepdata_lib

Library for getting your data into HEPData
https://hepdata-lib.readthedocs.io
MIT License
16 stars 39 forks source link

Add support for inhomogenous error breakdowns #265

Closed 20DM closed 4 months ago

20DM commented 4 months ago

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/

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | Coverage Δ | | |---|---|---| | [unittests-3.10](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.42% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-3.11](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.42% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-3.6](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.14% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-3.7](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.14% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-3.8](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.24% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-3.9](https://app.codecov.io/gh/HEPData/hepdata_lib/pull/265/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData) | `90.24% <100.00%> (+0.04%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HEPData#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GraemeWatt commented 4 months ago

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.

GraemeWatt commented 4 months ago

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

20DM commented 4 months ago

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. 🤷