search

scikit-hep / uproot3-methods

Pythonic behaviors for non-I/O related ROOT classes.
BSD 3-Clause "New" or "Revised" License
21 stars 28 forks source link

Writing general histograms (general how?) #51

Closed reikdas closed 5 years ago

reikdas commented 5 years ago

To be merged along with the corresponding uproot PR https://github.com/scikit-hep/uproot/pull/269

jpivarski commented 5 years ago

If this is introducing a new version dependency between uproot-methods and uproot, we'll need to give them both new semimajor version numbers (e.g. increase y in x.y.z). That will require a round of release candidates. Is the dependency between uproot-methods and uproot strictly required or can it be opt-in?

An alternate way of asking the question: if you remove unit tests specific to these two PRs, do all the tests pass? If so, maybe we should merge in the new features without new tests (so everything stays green, even as one thing gets deployed before the other) and once the dust settles, open new PRs to put in the tests.

reikdas commented 5 years ago

The commit - 65dd241583b411fc39c2dc705b24c02311d52be9(currently the second last commit) on the issue-137 branch of uproot, completely handles the new feature(writing TH2) - All the tests passed on that commit, apart from those with Python 3.6(there seems to be a dependency issue on Travis - https://travis-ci.org/scikit-hep/uproot/builds/523086187?utm_source=github_status&utm_medium=notification could you take a look?). So perhaps that could be merged in before the latest commit on the issue-137 branch(which requires this PR to be accepted for tests to pass) and this PR.

The generalization is that I am handling all sorts of histograms (currently TH1 and TH2, later TH3) in the currently named TH1.py file in uproot. This PR and the last commit in the corresponding uproot PR changes the name of that file and the class in the file to TH.

jpivarski commented 5 years ago

It turns out, we didn't need that. The pip install was just updating an apt install, which may be up to date by now anyway. I'll merge this. (Ack! And then I need to give it a new version number to do the deployment.)