Add support for writing 2 and 3 dimensional histograms #269

reikdas · 2019-04-22T16:16:59Z

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 · 2019-04-22T20:40:59Z

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

matthewfeickert · 2019-04-23T21:16:01Z

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 · 2019-04-24T03:27:41Z

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 · 2019-04-24T03:29:57Z

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 · 2019-04-24T05:14:46Z

@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 · 2019-04-24T07:24:11Z

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 · 2019-04-24T14:53:25Z

@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 · 2019-04-24T14:59:10Z

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 · 2019-04-24T21:11:15Z

@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 · 2019-04-24T22:54:37Z

uproot/write/objects/TH.py

        elif self.fClassName == b"TProfile3D":
            raise NotImplementedError(self.fClassName)
        else:
            raise ValueError("unrecognized histogram class name {0}".format(self.fClassName))

-        self.fields["_fNcells"] = len(self.valuesarray)
+        self.fields["_fNcells"] = self.valuesarray.size


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

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

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

@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 · 2019-04-25T00:37:56Z

@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- · 2019-04-25T02:07:10Z

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 · 2019-04-25T02:14:24Z

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

nsmith- · 2019-04-25T02:16:06Z

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

matthewfeickert · 2019-04-25T02:17:45Z

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

reikdas · 2019-04-25T03:22:31Z

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 · 2019-04-25T05:57:23Z

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 · 2019-04-25T06:00:05Z

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 · 2019-04-25T10:30:58Z

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.

The use of mamba is causing issues with Python 3.6 and some instances of Python 2.7. By using Conda this fixes the issue until mamba is more stable (and out of alpha stage)

reikdas · 2019-04-25T17:09:47Z

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.

…as less variance than sys.platform

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 · 2019-04-25T17:40:15Z

👍

jpivarski · 2019-04-25T18:25:15Z

Thank you both @reikdas @matthewfeickert!

reikdas mentioned this pull request Apr 22, 2019

Writing general histograms (general how?) scikit-hep/uproot3-methods#51

Merged

jpivarski changed the title ~~[WIP]Issue-137~~ [WIP] Writing 2-dimensional histograms Apr 22, 2019

reikdas changed the title ~~[WIP] Writing 2-dimensional histograms~~ [WIP] Writing 2 and 3 dimensional histograms Apr 22, 2019

reikdas changed the title ~~[WIP] Writing 2 and 3 dimensional histograms~~ Writing 2 and 3 dimensional histograms Apr 23, 2019

matthewfeickert mentioned this pull request Apr 23, 2019

uproot-methods v0.5.1 breaking pyhf scikit-hep/uproot3-methods#53

Closed

matthewfeickert force-pushed the issue-137 branch from 8efcf13 to 811f5d9 Compare April 24, 2019 05:17

matthewfeickert force-pushed the issue-137 branch from d1419a0 to 3ec9fce Compare April 24, 2019 13:39

matthewfeickert force-pushed the issue-137 branch 2 times, most recently from 344e906 to 4be5678 Compare April 24, 2019 20:33

matthewfeickert assigned matthewfeickert and reikdas Apr 24, 2019

matthewfeickert changed the title ~~Writing 2 and 3 dimensional histograms~~ Add support for writing 2 and 3 dimensional histograms Apr 24, 2019

matthewfeickert force-pushed the issue-137 branch from 4be5678 to 152c441 Compare April 24, 2019 21:51

matthewfeickert requested a review from jpivarski April 24, 2019 22:46

jpivarski approved these changes Apr 24, 2019

View reviewed changes

matthewfeickert force-pushed the issue-137 branch from 152c441 to b3ccf40 Compare April 25, 2019 01:09

reikdas added 4 commits April 24, 2019 22:16

Generalised TH1.py class to handle all histograms

a64b956

TH3 works

44628c8

Updated version

bd5411e

Cleaned code..

cdd0eba

matthewfeickert force-pushed the issue-137 branch from 5bac959 to 0200aa7 Compare April 25, 2019 03:35

reikdas force-pushed the issue-137 branch from 0200aa7 to 82be002 Compare April 25, 2019 05:02

reikdas force-pushed the issue-137 branch from 82be002 to b74a058 Compare April 25, 2019 12:32

reikdas added 10 commits April 25, 2019 18:02

Revert to using Conda instead of mamba for install

b74a058

The use of mamba is causing issues with Python 3.6 and some instances of Python 2.7. By using Conda this fixes the issue until mamba is more stable (and out of alpha stage)

Trying to fix occasional cachetools error

b92d70f

Update pip in appveyor

e0c0e3d

Skip test_hist_in_tree to fix appveyor

f44596f

Fix pytest skip syntax

3d06be2

Run pytest tests

540d63d

Remove setup.py from appveyor

f693648

Fix typo

49363df

Import pytest in test_tree.py

2b78d2a

Revert some appveyor changes

65d24e8

reikdas requested a review from jpivarski April 25, 2019 17:17

two places where Event.root is read over the network; also, os.name h…

e7c42fc

…as less variance than sys.platform

jpivarski approved these changes Apr 25, 2019

View reviewed changes

jpivarski merged commit 818e15d into master Apr 25, 2019

jpivarski deleted the issue-137 branch April 25, 2019 18:25

jpivarski mentioned this pull request Apr 25, 2019

Make XRootDSource fork-safe #270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for writing 2 and 3 dimensional histograms #269

Add support for writing 2 and 3 dimensional histograms #269

reikdas commented Apr 22, 2019 •

edited

Loading

reikdas commented Apr 22, 2019 •

edited

Loading

matthewfeickert commented Apr 23, 2019 •

edited

Loading

reikdas commented Apr 24, 2019

matthewfeickert commented Apr 24, 2019

matthewfeickert commented Apr 24, 2019

reikdas commented Apr 24, 2019 •

edited

Loading

matthewfeickert commented Apr 24, 2019

reikdas commented Apr 24, 2019 •

edited

Loading

matthewfeickert commented Apr 24, 2019

jpivarski left a comment

jpivarski Apr 24, 2019

reikdas Apr 25, 2019

matthewfeickert Apr 25, 2019

matthewfeickert Apr 25, 2019

matthewfeickert commented Apr 25, 2019

nsmith- commented Apr 25, 2019 •

edited

Loading

reikdas commented Apr 25, 2019

nsmith- commented Apr 25, 2019

matthewfeickert commented Apr 25, 2019

reikdas commented Apr 25, 2019 •

edited

Loading

reikdas commented Apr 25, 2019

matthewfeickert commented Apr 25, 2019

jpivarski commented Apr 25, 2019

reikdas commented Apr 25, 2019

jpivarski left a comment

reikdas commented Apr 25, 2019

jpivarski commented Apr 25, 2019

Add support for writing 2 and 3 dimensional histograms #269

Add support for writing 2 and 3 dimensional histograms #269

Conversation

reikdas commented Apr 22, 2019 • edited Loading

reikdas commented Apr 22, 2019 • edited Loading

matthewfeickert commented Apr 23, 2019 • edited Loading

reikdas commented Apr 24, 2019

matthewfeickert commented Apr 24, 2019

matthewfeickert commented Apr 24, 2019

reikdas commented Apr 24, 2019 • edited Loading

matthewfeickert commented Apr 24, 2019

reikdas commented Apr 24, 2019 • edited Loading

matthewfeickert commented Apr 24, 2019

jpivarski left a comment

Choose a reason for hiding this comment

jpivarski Apr 24, 2019

Choose a reason for hiding this comment

reikdas Apr 25, 2019

Choose a reason for hiding this comment

matthewfeickert Apr 25, 2019

Choose a reason for hiding this comment

matthewfeickert Apr 25, 2019

Choose a reason for hiding this comment

matthewfeickert commented Apr 25, 2019

nsmith- commented Apr 25, 2019 • edited Loading

reikdas commented Apr 25, 2019

nsmith- commented Apr 25, 2019

matthewfeickert commented Apr 25, 2019

reikdas commented Apr 25, 2019 • edited Loading

reikdas commented Apr 25, 2019

matthewfeickert commented Apr 25, 2019

jpivarski commented Apr 25, 2019

reikdas commented Apr 25, 2019

jpivarski left a comment

Choose a reason for hiding this comment

reikdas commented Apr 25, 2019

jpivarski commented Apr 25, 2019

reikdas commented Apr 22, 2019 •

edited

Loading

reikdas commented Apr 22, 2019 •

edited

Loading

matthewfeickert commented Apr 23, 2019 •

edited

Loading

reikdas commented Apr 24, 2019 •

edited

Loading

reikdas commented Apr 24, 2019 •

edited

Loading

nsmith- commented Apr 25, 2019 •

edited

Loading

reikdas commented Apr 25, 2019 •

edited

Loading