Add support for writing 2 and 3 dimensional histograms

[reikdas]

Uproot is able to write TH2s and TH3s.
This PR has to be merged with its corresponding PR in uproot-methods. (Also tests will not pass unless the uproot-methods PR has been merged)

[reikdas]

@jpivarski there seems to be a Travis issue with trying to install numpy on Python 3.6. Could you take a look?

[matthewfeickert]

there seems to be a Travis issue with trying to install numpy on Python 3.6. Could you take a look?

Hi @reikdas (coming from uproot-methods Issue 53). I see that this still seems to be an issue. Is this something that having more people looking at could help?

Though I don't think this is a Travis issue, but a Conda issue. The pip that is doing the installs is Conda's pip, not the Travis system Python runtime pip. So I think you'll find this issue if you run the test suite locally as well.

[reikdas]

there seems to be a Travis issue with trying to install numpy on Python 3.6. Could you take a look?

Hi @reikdas (coming from uproot-methods Issue 53). I see that this still seems to be an issue. Is this something that having more people looking at could help?

Yes! Thank you for your help!

[matthewfeickert]

Thank you for your help!

Sure, but don't thank me yet. ;) I've been poking at this as well and it seems to be a complicated issue (as you already well know).

[matthewfeickert]

@reikdas I'm going to do a rebase and force push to drop the commits you grabbed from my test branch. They have so far been unsuccessful and I think don't add anything helpful.

[reikdas]

Removing mamba fixed the issue.
But, I do not see how changing the .travis.yml file could make the appveyor tests fail. This is especially weird since appveyor/branch fails on Python2.7 and appveyor/pr fails on Python3.6. @jpivarski could you restart the appveyor jobs that failed?

[matthewfeickert]

@reikdas Forgive my interactive rebases and force pushes. I'm not trying to get in your way, but just squashing some commits in which necessary experimentation happens that isn't too relevant to the final PR's purpose. I can of course hold off on this until things are actually done, so I'll stop poking at that until the CI is all happy.

[reikdas]

just squashing some commits in which necessary experimentation happens that isn't too relevant to the final PR's purpose.

Thanks for doing that!

I can of course hold off on this until things are actually done, so I'll stop poking at that until the CI is all happy.

Perhaps, that would be best. The issue seems to have been solved by going back to conda but appveyor throws an error and the build stalls on travis for a particular instance of python 2.7.

[matthewfeickert]

@reikdas Hm, this is strange. The fix doesn't seem to be stable in all situations. Everything ran okay with your last push, but then I squashed commits and rebased against origin/master to make everything cleaner and more linear to follow (but the code is the same) and now we don't have fully stable builds anymore. Any thoughts on this strangeness?

Also if might be worth to do a git fetch and not pull to diff the results of your local branch vs. this one before you do anything to make sure that nothing subtle got changed. Let me know if I can unbreak anything that I might have caused.

[jpivarski]

I know that an awful lot of work went into this and I don't want to give a too cursory review, but this all is looking good to me.

I think we'll have to rethink the conda/mamba stuff. The faster builds were very nice, but correctness is most important.

I see various corrections to the testing and example-generating code, and each one looks right (as in, "of course; that was a typo").

I don't know about the wisdom of inventing a superclass TH that doesn't exist in ROOT. It already bit us in uproot-methods. In ROOT, TH1 is the superclass of 2 and 3 dimensional histograms, though that's a little nutty. Maybe that's a good enough argument for breaking pattern with the original ROOT hierarchy. (I made the same decision to simplify TFile/TDirectory/TDirectoryFile.)

I say it's good. There's not a lot to say beyond the fact that the tests are successful and the code is clean and easy to read.

Are the builds still unstable? Like, sometimes passing, sometimes failing? Or has that problem been fixed?

[jpivarski]

I prefer using len(array) rather than array.size, but I wouldn't make a big deal about it.

[reikdas]

Using len(array) instead of array.size would break the code for TH2 and TH3.

[matthewfeickert]

Yes, sorry about not thinking this through before shoving stuff @reikdas. I'm fixing this now.

[matthewfeickert]

@reikdas Okay, I have now unreverted the change. I am going to just leave this alone until tomorrow when there has been more time to debug the test suite.

I ran the test suit on my Linux machine and everything passed fine, so I'm confused as to what it going on here. :? Sorry for causing extra annoyances here.

[matthewfeickert]

@jpivarski I'll let @reikdas comment on the above as he was the one to do the work.

Are the builds still unstable? Like, sometimes passing, sometimes failing? Or has that problem been fixed?

With regards to this it seems that they are mostly stable, but there was some instances in which Appveyor would fail, but then when I would git commit --amend --no-edit && git push --force-with-lease origin issue-137 to force a restart of the build with the same code it would finish fine.

Let me implement your len(array) suggestion and we can see how that build fares.

[nsmith-]

So I see what appears to be a similar issue, but only for python 3.6 for any python, with:

Traceback (most recent call last):
  File "tmp.py", line 14, in <module>
    f["a histo"] = h
  File "/uscms/home/ncsmith/.local/lib/python3.6/site-packages/uproot/write/TFile.py", line 62, in __setitem__
    what = uproot_methods.convert.towriteable(what)
  File "/uscms_data/d3/ncsmith/dazsle/coffeandbacon/uproot-methods/uproot_methods/convert.py", line 50, in towriteable
    cls = getattr(importlib.import_module(mod), cls)
  File "/cvmfs/sft.cern.ch/lcg/releases/Python/3.6.5-56635/x86_64-slc6-gcc62-opt/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'uproot.write.objects.TH'

for the test script:

from uproot_methods.classes.TH1 import from_numpy
import uproot
import numpy as np
import os

outname = "test_output.root"

try:
    edges = np.array((0., 1., 2.))
    values = np.array([2, 3])
    h = from_numpy((values, edges))

    with uproot.create(outname) as f:
        f["a histo"] = h

finally:
    if os.path.exists(outname):
        os.remove(outname)

[reikdas]

@nsmith- It is the same issue as scikit-hep/uproot3-methods#53. It will be fixed once this PR has been merged.

[nsmith-]

Ok yeah I can confirm it doesn't fail if I checkout this branch, cool.

[matthewfeickert]

I can look into why things are failing again in about an hour.

[reikdas]

Also if might be worth to do a git fetch and not pull to diff the results of your local branch vs. this one before you do anything to make sure that nothing subtle got changed.

I made a lot of the travis-fixing commits using the web interface(since they were mostly removing a single line or a word), so this would probably not help. But, I took a look at the code and it seems to be the same. :)
Just an FYI for anyone watching this thread, I don't think this particular PR has anything to do with the builds being unstable and that any tests on the current master would bring out this issue.

[reikdas]

All the checks are passing currently. (Note that I don't think the travis build is stable yet, but I really do not know how to debug this - This seems like an appveyor issue.)

[matthewfeickert]

This seems like an appveyor issue

Yes, earlier it seemed like the appveyor errors were because appveyor was having trouble downloading ROOT files before timing out/giving up, and not that the code itself was the issue.

[jpivarski]

AppVeyor had trouble downloading Event.root, so I put in a skip for that test (it also took a long time). If you know which ones are failing for the download, we can just put in a skip for them (the explicit @pytest.skip, no that it shows up in the report). I know that it's not ideal, but if we know that the test isn't failing for an uproot reason, just network connectivity, then we can lose a little coverage for stability (and potentially address it later).

I'm surprised about len(array) vs array.size. I'll have to learn what's the difference between them.

[reikdas]

If you know which ones are failing for the download, we can just put in a skip for them (the explicit @pytest.skip, no that it shows up in the report).

Skipping the occasionally failed test (and a few minor changes in the appveyor config file) seems to have fixed the issue.

[jpivarski]

This is good: the test-skipping is localized to Windows because of AppVeyor, so it still runs in most of the matrix (on Travis). I changed the sys.platform check to os.name because the latter is less variable, and moved the imports to the top of the file. Also, there were two places where Event.root is read over the network; you were just lucky last time in that the other one didn't fail.

I think this PR is ready for merging (thank you for all your hard work!). Give me a quick response so that I know you agree.

[reikdas]

👍

[jpivarski]

Thank you both @reikdas @matthewfeickert!