Skip to content

Conversation

@beojan
Copy link
Contributor

@beojan beojan commented May 4, 2021

This adds a behaviour for TGraph (based on that for TGraphAsymmErrors), and behaviours for RooHist and RooCurve based on these. These make it easier to plot RooFit objects on a plot that's otherwise made with Python.

Draft because I haven't added any tests yet. I thought it would be better to discuss first.

@beojan beojan mentioned this pull request May 4, 2021
@jpivarski jpivarski linked an issue May 4, 2021 that may be closed by this pull request
@jpivarski jpivarski marked this pull request as draft May 4, 2021 18:24
@jpivarski jpivarski changed the title Draft: Add TGraph, RooCurve, and RooHist behaviours Add TGraph, RooCurve, and RooHist behaviours. May 4, 2021
@jpivarski
Copy link
Member

It passes tests, but let me know when it's out of "draft" status (when you think it's ready for merging).

A comment: I'd prefer to not introduce new specialized exception types; it looks like both of these could be ValueError. I know that having more exception types can be good for writing more fine-grained try-catch logic, but it's hard to be consistent in defining exception types for different things; the system tends to get lopsided with specialized exceptions for one thing and not others. Uproot has a few specialized exceptions (subclasses of the exceptions you'd expect, such as KeyInFileError for KeyError), but all of those cases involve getting more information to users. (KeyInFileError gives a list of other keys, sorted by string difference from the one you gave; DeserializationError is part of a try, try-again-with-TStreamerInfo pattern, etc.)

@jpivarski
Copy link
Member

Let me know when this is done!

@beojan
Copy link
Contributor Author

beojan commented May 22, 2021

Sorry, I was a little busy with other things. Here's the test file I'd like to use:

test-rooplot.zip

@jpivarski
Copy link
Member

If you add that file in the directory below with name "uproot-issue-350.root", we can make a real test around it:

https://github.com/scikit-hep/scikit-hep-testdata/tree/main/src/skhep_testdata/data

I'll approve that scikit-hep-testdata PR and make a new version quickly so that you can test it here.

@beojan
Copy link
Contributor Author

beojan commented Jun 5, 2021

PR made here. Sorry for the delay.

@jpivarski
Copy link
Member

@beojan, uproot-issue-350.root is now available in scikit-hep-testdata 0.4.4. Tests using it should now work!

@beojan
Copy link
Contributor Author

beojan commented Jun 21, 2021

Tests now added. It looks like it failed because of a conda issue though.

@jpivarski
Copy link
Member

I restarted the tests, and they're failing in a relevant part now:

______________________ ERROR at setup of test_interpolate ______________________
file /home/runner/work/uproot4/uproot4/tests/test_0350-read-RooCurve-RooHist.py, line 117
  def test_interpolate(roocurve, roocurve_err):
E       fixture 'roocurve' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, datafile, doctest_namespace, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, reset_classes, roocurve_err, roocurve_hist, roohist, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

Maybe you've set up the new fixture locally but haven't checked it in?

(Unless it simplifies things in a noticeable way, I don't think it's necessary to use a lot of pytest features, such as fixtures. For instance, is this fixture used in many places, such that replacing redundancy with indirection is a good trade-off? I generally see tests as requiring a lower level of DRY than the main codebase, since the majority of them are written once and never updated, whereas the main codebase is frequently operated upon.)

beojan added 2 commits June 21, 2021 17:45
That's what's used in the test definitions
@jpivarski
Copy link
Member

_____________________________ test_interpretation ______________________________

roohist = <RooHist (version 1) at 0x7f66e4954650>
roocurve = <RooCurve (version 1) at 0x7f66fc0707d0>
roocurve_err = <RooCurve (version 1) at 0x7f66d7f49c10>

    def test_interpretation(roohist, roocurve, roocurve_err):
        assert roohist.classname == "RooHist"
>       assert roohist.behaviors[0] == uproot.behaviours.RooHist.RooHist
E       AttributeError: module 'uproot' has no attribute 'behaviours'

tests/test_0350-read-RooCurve-RooHist.py:38: AttributeError

Needs an explicit import uproot.behaviors.RooHist in the testing file. Behaviors aren't loaded preemptively because they might grow to be a large amount of code someday.

________________________________ test_to_boost _________________________________

roohist = <RooHist (version 1) at 0x7f66d75c7890>
roocurve = <RooCurve (version 1) at 0x7f66d75d6910>
roocurve_err = <RooCurve (version 1) at 0x7f66d75cf910>

    def test_to_boost(roohist, roocurve, roocurve_err):
        rh_boost = roohist.to_boost()
>       assert rh_boost.axes[0].edges == numpy.arange(0.0, 51.0)
E       ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

tests/test_0350-read-RooCurve-RooHist.py:51: ValueError

Could use NumPy's array_equal, or convert both sides with tolist, or use all as they suggest.

@jpivarski
Copy link
Member

(I'm going offline now. Might be unresponsive; sorry!)

@beojan
Copy link
Contributor Author

beojan commented Jun 21, 2021

Needs an explicit import uproot.behaviors.RooHist in the testing file.

That wasn't it (I copied the TGraphAsymmErrors test). I just spelled "behaviors" the British way.

I've also fixed the other test.

@beojan beojan marked this pull request as ready for review June 22, 2021 13:14
@beojan
Copy link
Contributor Author

beojan commented Jun 22, 2021

All tests passed. I think this is ready for review.

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.

This looks absolutely great, thank you very much!

@jpivarski jpivarski merged commit 511f2d6 into scikit-hep:main Jun 22, 2021
@beojan beojan deleted the tgraph-and-roohist branch June 22, 2021 13:24
@beojan
Copy link
Contributor Author

beojan commented Jun 25, 2021

By the way, are you planning to cut a new release soon? It's not urgent (I can just build from git), I'm just curious.

@jpivarski
Copy link
Member

I am. A lot of things have been coming in on Awkward and I'm looking for a good gap in which to cut a release there, and I would then do both, but there's no reason I shouldn't do it for Uproot now.

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.

TGraph not supported

2 participants