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
Conversation
There was a problem hiding this 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.
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 ). |
@jpivarski maybe I'll leave this here for now. I thought you would've had python |
I suppose The primary place for test files is As far as I know, Uproot has been the only project using 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 |
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. |
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 is the forth most popular package https://pythonwheels.com , I certainly wouldn't worry about it as a dependency, especially for tests. :) |
For completeness - also |
Indeed. My only issue is that this file is not a good candidate for a common test utility for multiple packages. I tend to agree with @jpivarski that it feels a lot more cumbersome to add to the |
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. |
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. |
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. |
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.) |
@all-contributors please add @kratsg for code |
I've put up a pull request to add @kratsg! 🎉 |
here's a test script to work with