Miscellaneous usability issues

[nsmith-]

Even the best laid plans won't survive first contact with the user. Let me know if you want these broken into separate issues, but most are small. p.s. this is with v0.5.2

I installed the package and played around with it, first reaction: docstrings are quite sparse, probably due to the large amount of pybind11 shenanigans.
It would be nice if the various axis type inheritances are visible from python, or at least something that tells me isinstance(x, bh.axis.axis) or something to that effect.

1 (see #216)

First dumb move:

ax = bh.axis.regular(20, 0, 1)
ax.value()

raises a cryptic TypeError, but I just didn't know the function required arguments.

2 (see #215)

For growable categories, I hit several obstacles:

ax = bh.axis.category([], growth=True)

raises ValueError saying there must be at least one bin.
Is it not possible to create growable axis with empty starting categories?

3

Answered well by #214 (comment)

Playing with indexing,

ax = bh.axis.category(['old'], growth=True)
ax.index('new')
# returns 1
ax.extent
# returns 1, why?

so I assume we should only treat axis objects as immutable with no actual state w.r.t. filling?
Of course, if I make a histogram using this axis, everything works as expected:

h = bh.histogram(bh.axis.category(['old'], growth=True))
h.fill(['hi', 'there'])
h.axis(0).extent
# returns 3

4 (see #184)

The options for how to provide indices to fill are a bit weird:

h = bh.histogram(bh.axis.category(['old'], growth=True))
h.fill('hi')

raises ValueError about casting strings to char const&, so I cannot use plain strings?

4.a (#230)

I see the choice to interpret python strings as iterables causes some headache:

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill('hi', np.arange(4))

raises ValueError: spans must have compatible lengths
(although even when they are compatible, the string to char array issue comes up again)

5 #233

Ok, so sticking to arrays,

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill(['hi'], np.arange(4))
Segmentation fault: 11

uh oh!
but this works ok:

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill(['hi'], np.arange(1))

5.a #230 (not supported)

How about numpy scalars (dimension-0 arrays)?

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill(np.array('hi'), np.arange(3))

raises a cryptic ValueError: allocator<T>::allocate(size_t n) 'n' exceeds maximum supported size

But indeed numpy 1D arrays work:

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill(np.repeat('hi', 3), np.arange(3))
h.fill(np.array(['hi']), np.arange(3))

so broadcasting is supported, nice!

6

When adding histograms, the growable categories are not as flexible as I'd like:

h = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h2 = bh.histogram(bh.axis.category([''], growth=True), bh.axis.regular(20, 0, 1))
h.fill(np.array(['hi']), np.array([.1]))
h2.fill(np.array(['hi again']), np.array([.2]))
h + h2

raises ValueError: axes of histograms differ, when clearly one could be grown to accept the other
I tried a workaround:

h.fill(np.array(['hi', 'hi again']), np.array([0, 0]), weight=np.zeros(2))
h2.fill(np.array(['hi', 'hi again']), np.array([0, 0]), weight=np.zeros(2))
h + h2

which also fails since the categories are introduced in a different order in the respective histograms.

7

About categorical axes, it looks like the storage contains the outer product of growable categories:

h = bh.histogram(
  bh.axis.category([''], growth=True),
  bh.axis.category([''], growth=True),
  bh.axis.category([''], growth=True),
  bh.axis.regular(60, 60, 120),
)
h.fill(
  ['dataset1', 'dataset2', 'dataset2'],
  ['region1', 'region1', 'region2'],
  ['', '', 'JESup'],
  [90., 86., 92.],
)
import pickle
len(pickle.dumps(h))

returns 9290 (787 empty), while for comparison,

import coffea.hist as hist
h = hist.Hist('events',
  hist.Cat('dataset', ''),
  hist.Cat('region', ''),
  hist.Cat('systematic', ''),
  hist.Bin('mass', '', 60, 60, 120),
)
h.fill(dataset='dataset1', region='region1', systematic='', mass=90.)
h.fill(dataset='dataset2', region='region1', systematic='', mass=86.)
h.fill(dataset='dataset2', region='region2', systematic='JESup', mass=92.)
len(pickle.dumps(h))

returns 4898 (2880 empty). Coffea histograms have a fair bit of pickling overhead compared to boost, but the sparseness catches up.
Is there a way to request sparse bin storage here?

[henryiii]

This is fantastic, thank you so much! I'll go into working through each of these after #200 is completed, but let me say a few things. Lots of mechanical things have improved in develop; doc strings are getting longer (not ideal yet, but much better), signatures are better, and the PyBind11 interface is hidden behind a wrapping layer, so we have explicit control over error messages. I've been doing a lot of work on the inheritance tree in Python recently, as well.

We used to have a way to change an axis (update), but it was removed. While it was useful, it probably had limitations (the meaning of return values being the one I remember; maybe a storage problem if it was also used by a histogram). @HDembinski can say more about whether it will ever come back. Otherwise, axes are immutable except inside histograms. I agree it would be instructive to be able to play with them like this. Maybe we can do something more explicit when an invalid index into a growable axis is requested, like a helpful error or such; @HDembinski ?

The problem with single strings is known, see at least #184, though I think you are showing that it extends further than that. We need to be consistent here. For now, strings always need to be in [].

A category axis comes in integer and string forms, so making an empty one is indeterminate; which do you want, string or int? We probably need to try to solve this.

[henryiii]

1

>>> ax = bh.axis.Regular(20, 0, 1)
>>> ax.value()
TypeError: value() missing 1 required positional argument: 'i'

This is what we get in develop. Probably should have a better name for the bin number argument than i? How about bin and value (depending one which one the methods take)? @HDembinski ?

I've added numbers on your issue to make it easier to work through and discuss. Feel free to modify if I didn't capture the key points.

[nsmith-]

The numbers look good. Thanks for splitting, I wasn't sure how granular to make it.
For 1, I would go with ax.value(index=...) and ax.index(value=...) as they are (I think) a bijection (well, after the floor operation)

[HDembinski]

@nsmith- Thank you for giving the boost-histogram beta a spin and sending this nice feedback, but could you in the future please make separate items for separate issues? Discussing about these now is quite difficult, there is too much going on in this thread.

1. I like @nsmith- proposal.

2. We are working on this, good catch.

3. The index method never changes the axis, therefore it cannot grow it. In C++, there is a special method called update which grows it. We are not exposing the update method in Python, because if you could call the update method of an axis attached to a histogram, you could change the axis without the histogram knowing about it. The histogram has to know about it, because it has to grow its storage, too. In C++, I can make sure that the update method cannot be called, because you can only get a const reference of an axis attached to a histogram and the update method is not marked as const (since it mutates the axis). But Python does not distinguish between a value and a const references automatically. We would have to implement all axis classes two times in Python, once as a value and once as a const reference. All that just so you can call the update method. I think this is not worth it, so we just don't expose it.

4), 4a), 5), 5a) these are bugs and will be fixed. The idea is that fill does broadcasting, so you are allowed to mix arrays and values, the values will be broadcasted to the same size as the arrays. Passing arrays of different lengths is an error. So 4a) and 5a) should work, the string should be broadcast to an array of strings. 5) should give you an error about passing arrays with different lengths.

6. This is a very interesting point. You currently cannot add histograms which have different axes, therefore you get this error, but I see your point, probably this should actually work and do what you suggest, when the axes are growable. I need to think about it more, but I like your proposal.

7. Sparse storages are implemented in boost::histogram (C++), but currently there are no wrappers in boost-histogram (Python wrapper). Histograms with sparse storage are neither performant nor memory efficient. A sparse storage needs lots of memory overhead for each cell that is filled and cell lookup is much slower, too. Not everything that sounds good in theory also works well in practice. Could you run your coffea histogram with sparse storage against boost-histogram with dense storage on a realistic data set?

[HDembinski]

For reference: https://stackoverflow.com/questions/9375450/stdunordered-map-very-high-memory-usage

[HDembinski]

A pickled sparse container is much smaller than its representation in memory.

[HDembinski]

On more word on sparse storage: we should benchmark with this one https://github.com/greg7mdp/sparsepp. It is supposed to be a drop-in replacement for unordered_map, and so it should work out of the box with boost::histogram.

[nsmith-]

So I'd let Henry confirm, but sounds like all but # 6 and 7 have issues tracking them, so I can split those into two new ones and close this?

[henryiii]

That's fine with me, though you can also leave this one open till all the related issues are done. I'll probably want to run through the examples to make sure 4-5 get fixed.