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

TGraph not supported #309

Closed
beojan opened this issue Mar 22, 2021 · 9 comments · Fixed by #350
Closed

TGraph not supported #309

beojan opened this issue Mar 22, 2021 · 9 comments · Fixed by #350
Labels
feature New feature or request

Comments

@beojan
Copy link
Contributor

beojan commented Mar 22, 2021

While uproot4 supports TGraphAsymmErrors, it seems plain TGraph isn't supported.

@beojan beojan added the bug (unverified) The problem described would be a bug, but needs to be triaged label Mar 22, 2021
@jpivarski jpivarski added feature New feature or request and removed bug (unverified) The problem described would be a bug, but needs to be triaged labels Mar 22, 2021
@jpivarski
Copy link
Member

"Support" in this case means, "has special high-level methods for easier access." All of the data can be accessed as all_members (dict) and member (method to access one).

TGraphAsymmErrors has these high-level methods because @kratsg had an idea about how he wanted to access TGraphAsymmErrors and implemented it (#240). The equivalent for TGraph would probably be very similar: a values property without the errors.

It's enough to add a submodule with the class name containing a class of the same name in uproot.behaviors, similar to

https://github.com/scikit-hep/uproot4/blob/main/uproot/behaviors/TGraphAsymmErrors.py

Since there are (to a first approximation) infinitely many classes in ROOT and the automated interpretation gets all of the private data members into all_members, I'd like to encourage users of Uproot to define how they want to access their favorite classes. That's why "uproot-methods" used to be a separate module (so that it could change more rapidly than the main codebase, and demonstrably didn't break core functioning), but keeping it as a separate module led to it getting ignored and out-of-date. So the methods/behaviors are part of the main Uproot codebase again, but I'd still like users of these classes to take an active part in shaping them.

@kratsg
Copy link
Contributor

kratsg commented Mar 22, 2021

I did implement being able to read memberwise versions of TGraph recently in #298 . I think until that point, it wasn't quite easy for one to be able to read it in. One could use the test file in scikit-hep to write a test for it. I'm guessing because it's a TGraph, you'd only want the x/y values and not much else?

@beojan
Copy link
Contributor Author

beojan commented Mar 22, 2021

In my case I'm actually reading a RooCurve. I'll work on a PR for this, the RooCurve could use some extra functions to disentangle the strange way RooFit plots histograms and errors on histograms.

@jpivarski
Copy link
Member

Wait, are we talking about deserializing (to get an object at all) or accessing the object's methods in an easier way? Deserialization is a more significant effort (and @kratsg's work on memberwise deserialization is beyond what I'd expect most people to be able to do).

I thought we were talking about the fact that there's a TGraphAsymmErrors submodule in uproot.behaviors but not a TGraph.

@beojan
Copy link
Contributor Author

beojan commented Mar 22, 2021

I was talking about the behaviour. I can see that we have a filled all_members already.

@kratsg
Copy link
Contributor

kratsg commented Mar 22, 2021

Well, I believe up until that PR -- most TGraphs were not able to be read in since it seems a lot of them tend to be memberwise-serialized. Now that they can be read in, it shouldn't be too hard to add in the behavior. Just mostly explaining why the behavior never quite existed before for a common object.

@jpivarski
Copy link
Member

Well, "most" depends on what kinds of ROOT files you tend to look at (i.e. the measure of your space). I remember that uproot-methods had a behavior defined for TGraph, indicating that Uproot 3 could access TGraphs, even though Uproot 3 didn't do any memberwise deserialization.

It was scikit-hep/uproot3-methods#7, added by @marinang. Looking at that now, I see that there's actually three classes, TGraph, TGraphErrors, and TGraphAsymmErrors. His implementation had a lot of methods for finding the maximum and such, though I'd rather have these objects have smaller interfaces that go straight to the library with all of that functionality now. That is, with a values method to get a NumPy array, things like the maximum can be computed in NumPy. There was also a matplotlib method; it wouldn't be bad to have something that immediately returns an "ax" to be mixed into a plot or implicitly drawn by Jupyter.

Actually, TGraph and TGraphErrors are the only two behaviors that haven't been reimplemented in Uproot 4 or are going to be in Vector. That set—histograms/profiles, graphs, TParameter, and 2D/3D/Lorentz vectors—are the ones that are desirable enough that they were requested again after the Uproot 3 → 4 transition. THnSparse is in the old uproot3-methods/classes directory, but it's just a placeholder (says "hello world") as an illustration of how to get started.

@beojan
Copy link
Contributor Author

beojan commented May 4, 2021

I've made a PR here with a draft implementation. No tests yet, I think those need to go in scikit-hep-testdata, right?

@jpivarski jpivarski linked a pull request May 4, 2021 that will close this issue
@jpivarski
Copy link
Member

@beojan Thank you!

Any files that are needed as input to a test would have to go into scikit-hep-testdata/src/skhep_testdata/data, which has a normal PR/release process. If you have any files that you plan to use to test #350, you can make the PR now and I'll merge-and-deploy scikit-hep-testdata as soon as possible. It should be publicly sharable and not much larger than a megabyte. (The largest so far is 64 MB, but the median is 68 kB.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants