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: Add TGraphAsymmErrors #240

Merged
merged 14 commits into from Jan 11, 2021
Merged

feat: Add TGraphAsymmErrors #240

merged 14 commits into from Jan 11, 2021

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Jan 8, 2021

here's a test script to work with

import uproot
import os.path

fname = "HEPData-ins1755298-v3-Expected_limit_1lbb.root"

if not os.path.isfile(fname):
    with open(fname, 'wb') as f:
        import requests
        response = requests.get("https://www.hepdata.net/download/table/ins1755298/Expected%20limit%201lbb/3/root")
        response.raise_for_status()
        f.write(response.content)

f = uproot.open(fname)
graph = f['Expected limit 1lbb/Graph1D_y1']
graph.values()
graph.errors()

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Great, thanks! After reading this over a second time, I have some suggestions for the API, described below.

uproot/behaviors/TGraphAsymmErrors.py Outdated Show resolved Hide resolved
uproot/behaviors/TGraphAsymmErrors.py Outdated Show resolved Hide resolved
uproot/behaviors/TGraphAsymmErrors.py Outdated Show resolved Hide resolved
@jpivarski
Copy link
Member

here's a test script to work with

Oh, and if that URL is stable, it can be included as a test in Uproot's test suite (numbered with "0240" to reference this PR). It just needs to be marked as "network" (like this) so that it can be turned off if someone doesn't have network access.

@kratsg kratsg marked this pull request as ready for review January 9, 2021 01:58
@kratsg
Copy link
Contributor Author

kratsg commented Jan 9, 2021

Oh, and if that URL is stable, it can be included as a test in Uproot's test suite (numbered with "0240" to reference this PR). It just needs to be marked as "network" (like this) so that it can be turned off if someone doesn't have network access.

I've gone ahead and added a full-fledged network-based test for this. We definitely should do something similar for pyhf for network-related tests (if we don't already @matthewfeickert ).

@kratsg
Copy link
Contributor Author

kratsg commented Jan 9, 2021

@jpivarski maybe I'll leave this here for now. I thought you would've had python requests as part of the test suite. HEPData doesn't allow streaming or chunking ROOT files (I tried) and you just need to download the file entirely which is how I wrote the test... do you want to add requests as a test dependency? 🤷‍♂️

@jpivarski
Copy link
Member

I suppose requests could be a test dependency. I had been avoiding it in the main codebase because I wanted to find out what we're paying for with that dependency. (One thing is the automated handling of redirects, but that was easy enough to add to the built-in Python HTTP client.)

The primary place for test files is scikit-hep-testdata, which get installed as local files, but the process of adding a file to that repo and pushing a new version of it all before you can test a PR here is a bit arduous. We should probably find a way to distribute local files for tests in a way that doesn't involve PyPI. But that's a longer term project.

As far as I know, Uproot has been the only project using scikit-hep-testdata, but if you have similar needs in pyhf, we could maybe band together to make a common test file source (accessible as both HTTP and local, maybe even XRootD, without file size limits and without the steps involved in publishing versions, since the files rarely/never change, they just get added to).

I have a big collection of files on my computer named "uproot4-lazy", which are files associated to Uproot 4 issues that I have been too lazy to add to scikit-hep-testdata. They're each about a MB or less.

@kratsg
Copy link
Contributor Author

kratsg commented Jan 9, 2021

As far as I know, Uproot has been the only project using scikit-hep-testdata, but if you have similar needs in pyhf, we could maybe band together to make a common test file source (accessible as both HTTP and local, maybe even XRootD, without file size limits and without the steps involved in publishing versions, since the files rarely/never change, they just get added to).

We certainly do have similar needs in pyhf. I think we're going to start seeing this needed for ServiceX as well and various other IRIS-HEP projects. I wonder if it's possible to talk with the Chicago folks here about having something like faxbox available for use. I know we've used CVMFS in the past to unpack and store various docker images so that grid sites only need CVMFS in order to access a subset of docker images (if they are registered there). One concern i've had generally is that this decouples the files from the codes that use it, and it's much harder to figure out who depends on a file when it needs to get updated or changed, or what that policy would be.

This file isn't abundantly large and I could get a pure-python only variation of the setUp fixture here in order to make this work (based on how I factorized the tests) so it's not the end of the world here. That said, i've always thought test dependencies were mostly not as restrictive since only the CI and developers would need to grab them. I guess you're trying to improve the developer experience here.

@jpivarski
Copy link
Member

That said, i've always thought test dependencies were mostly not as restrictive since only the CI and developers would need to grab them.

That's what I'm saying: there's a much lower bar to including a dependency for the tests, and I'm definitely willing to make requests a test dependency.

@henryiii
Copy link
Member

henryiii commented Jan 9, 2021

Requests is the forth most popular package https://pythonwheels.com , I certainly wouldn't worry about it as a dependency, especially for tests. :)

@eduardo-rodrigues
Copy link
Member

As far as I know, Uproot has been the only project using scikit-hep-testdata, but if you have similar needs in pyhf, we could maybe band together to make a common test file source (accessible as both HTTP and local, maybe even XRootD, without file size limits and without the steps involved in publishing versions, since the files rarely/never change, they just get added to).

For completeness - also pylhe uses it. For now I believe it only contains in there a single test file, though. So that's 3 org packages using the test package, which is nice.

@kratsg
Copy link
Contributor Author

kratsg commented Jan 11, 2021

For completeness - also pylhe uses it. For now I believe it only contains in there a single test file, though. So that's 3 org packages using the test package, which is nice.

Indeed. My only issue is that this file is not a good candidate for a common test utility for multiple packages. uproot has the slightly special case that while it's great for it to test lots of "common" ROOT files, it also needs to handle edge cases as well as minimal cases which are not of as-interest for other packages using the scikit-hep-testdata. There's a similar issue with pyhf because we only want HistFactory objects in these files which isn't that useful for something like uproot at the moment.

I tend to agree with @jpivarski that it feels a lot more cumbersome to add to the scikit-hep-testdata repo for something like this.

@eduardo-rodrigues
Copy link
Member

Yes. Do keep in mind that the test files can also be handy for tutorials, sometimes. And some (a very small subset) have been used in tutorials and hands-on. For example the pylhe test file could also be useful for pyhepmc ...

I'm not saying the situation of having to make a release is perfect.

@henryiii
Copy link
Member

Ideally, we should haven an online repo of files, and scikit-hep-testdata would just be an interface to that online repository, save for a few, tutorial-ready files locally.

@henryiii
Copy link
Member

It could be modeled after importlib.resouces.files in Python 3.9, where it returns an object that behaves like a Path opens the remote resource.

@kratsg
Copy link
Contributor Author

kratsg commented Jan 11, 2021

fyi @jpivarski this is ready to go.

@jpivarski
Copy link
Member

fyi @jpivarski this is ready to go.

Thanks! I just added some formatting and documentation and will merge it when it passes. (I also manually tested Python 2.7.)

@jpivarski jpivarski merged commit f67c67c into main Jan 11, 2021
@jpivarski jpivarski deleted the feat/tgraphasymmerrors branch January 11, 2021 23:13
@jpivarski
Copy link
Member

@all-contributors please add @kratsg for code

@allcontributors
Copy link
Contributor

@jpivarski

I've put up a pull request to add @kratsg! 🎉

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.

None yet

4 participants