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.

Sign up for GitHub

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

Jump to bottom

Converting from scikit-hep histogram to Table #243

Merged
merged 12 commits into from
Nov 15, 2023
Merged

Converting from scikit-hep histogram to Table #243

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3425353
Added reader and help functions to convert scikit-hep histogram to Ta…
yimuchen Oct 27, 2023
b6a0d9a
Updated documentation and dependencies
yimuchen Oct 27, 2023
6ab8cb6
Fixing complaints from pylint
yimuchen Oct 30, 2023
7ddb455
Added unit test for hist_utils functions
yimuchen Oct 30, 2023
ab99f26
Updating dependencies
yimuchen Oct 30, 2023
67a9ad4
Expliciltly listing all dependencies
yimuchen Oct 30, 2023
6ae8b03
Fixed pylint for test scripts
yimuchen Oct 30, 2023
a9783ee
Disabling specific pylint
yimuchen Oct 30, 2023
bab4364
Support for all standard hist.storage methods. Testing for all storag…
yimuchen Oct 30, 2023
da1740a
Additional edge cases handling
yimuchen Nov 3, 2023
11ae275
Generalizing storage method detector to work for any input (compatibi…
yimuchen Nov 3, 2023
f7dcb33
Added additional handling for integer bins
yimuchen Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions tests/test_histread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ def test_default_read(self):
"""
try:
readout = read_hist(TestHistUtils.base_hist)
# pylint: disable=W0702
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using this try-except block here (and also below) in the first place? Does it ever fail? If so, you should know which kind of exception is thrown and use that. Please do not disable pylint warnings.

Copy link
Contributor Author

@yimuchen yimuchen Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is following the format that was found in cfilereader and other tests [1].

Hm... I guess since the format parsing is pure python. I should actually remove the try/except all together, and simply have pytest handle the exception?

[1] https://github.com/HEPData/hepdata_lib/blob/main/tests/test_cfilereader.py#L48

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK, I had forgotten that this is used elsewhere. I think also in https://github.com/HEPData/hepdata_lib/blob/main/tests/test_cfilereader.py#L48 it should actually not be used, but let's not worry about that here.
I'm wondering what test_default_read is meant to test. Can read_hist(TestHistUtils.base_hist) actually fail? I would think that it can only happen if the hist.Hist() interface changes. So I would think that you can just read in the file and get rid of the try-except. However, there might be other ways that reading fails. For example, is it possible that an invalid hist is provided?

Copy link
Contributor Author

@yimuchen yimuchen Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The point of the test function is to check if axis information is properly formatted into compatible lists (hepdata_lib basically wants everything to be in 1D list, so all axis information needs to be multiplied out)

Technically, yes, the read_hist function can fail, as it depends on the storage format of the histogram entries [1], which can be user defined. For completeness’ sake, let me work on getting all official storage formats [2] into the package, and just have the function throw an error for user-defined formats for now.

[1] https://github.com/yimuchen/hepdata_lib/blob/hist_reader/hepdata_lib/hist_utils.py#L39
[2] https://github.com/scikit-hep/hist/blob/main/src/hist/storage.py#L14

except:
self.fail("Histogram reading raised an unexpected exception.")
# pylint: enable=W0702

# Checking dimension compatibility
self.assertTrue(len(readout["dataset"]) == len(readout["flavor"]))
Expand All @@ -67,8 +69,11 @@ def test_projection_read(self):
TestHistUtils.base_hist[{"dataset": "data", "flavor": sum}]
)
read2 = read_hist(TestHistUtils.base_hist[{"dataset": "QCD", "flavor": 0j}])
# pylint: disable=W0702
except:
self.fail("Histogram reading raised an unexpected exception.")
# pylint: enable=W0702

# Checking dimension compatibility
self.assertTrue(len(read1["eta"]) == len(read1["pt"]))
self.assertTrue(len(read1["eta"]) == len(read2["pt"]))
Expand Down Expand Up @@ -106,8 +111,10 @@ def test_uncertainty_generation(self):
"asymmetric histogram": (q_h, t_h),
},
)
# pylint: disable=W0702
except:
self.fail("Unexpected exception of automatic uncertainty generation.")
# pylint: enable=W0702

def check_val(arr1, arr2):
return self.assertTrue(
Expand Down Expand Up @@ -157,8 +164,10 @@ def test_table_generation(self):
axes_rename=_rename_,
axes_units=_units_,
)
# pylint: disable=W0702
except:
self.fail("Unexpected exception of table generation.")
# pylint: enable=W0702

readout = read_hist(TestHistUtils.base_hist)

Expand Down
Loading