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

uproot-methods v0.5.1 breaking pyhf #53

Closed matthewfeickert closed 5 years ago

matthewfeickert commented 5 years ago

Hi @jpivarski, in the pyhf test suite we're seeing problems caused by uproot-methods release v0.5.1 (The test-suite passes if I pin uproot-methods at v0.5.0). The main culprit seems to be

E   ModuleNotFoundError: No module named 'uproot.write.objects.TH'

I'm taking a look at this now, but I just wanted to bring it to your attention.

matthewfeickert commented 5 years ago

Ah, I now see that this is because uproot PR 269 hasn't been finished. I'll follow up there.

jpivarski commented 5 years ago

You'll probably follow up where @reikdas sees this, but I'll point him to it anyway.

Formally, we should have done another semimajor version (x.y.0) and started a series of release candidates, but I thought that this change only affected new features that aren't public yet. It had escaped my attention that TH is an uproot class that doesn't exist yet, rather than a ROOT class (that would come into being through streamers). I see the mistake now.

Thanks for catching this. I think that it could be fixed by including a check for TH or TH1 with a resolution order that doesn't touch the nonexistent class (i.e. only checking for a name string match, which I think the affected code does anyway, to avoid loading a bunch of modules just to check for their types).

reikdas commented 5 years ago

I knew this could cause a problem and was hoping to have the uproot pull request merged in along with the pull request that broke pyhf but I didn't see me running into that unforeseen error with the tests :(

matthewfeickert commented 5 years ago

I didn't see me running into that unforeseen error with the tests

@reikdas I think we've all been in this situation before. :) I'm sure we'll get it fixed soon enough, so don't worry about it. :+1:

matthewfeickert commented 5 years ago

Resolved by uproot PR 269 and release of uproot v3.5.1. Many thanks @reikdas!