[BUG] (Discussion) Input validation/preprocessing in AxesTuple

[jonas-eschle]

Describe the bug

Currently, AxesTuple inherits the constructor behavior of tuple in that it requires an iterable. While this makes sense and is consistent, it can create a hard-to-spot bug introduced by the fact that a single axis is also iterable and therefore if not an iterable of one or more axes but a single axis is given, it won't fail on instantiation. However, it will fail when trying to use any attribute.

I would propose to modify the constructor to be "axis-like" aware and convert a single axis automatically to an iterable with a single axis first.

The reason why I think it's fine to do is because the intention of AxesTuple(axis1) will always be actually AxesTuple([axis1]).

Alternatively, this could just error when constructing the AxesTuple

Or what do you think?

Steps to reproduce

import boost_histogram as bh

axis1 =  hist.axis.Regular(50, -5, 5)

axes = bh.axis.AxesTuple([axis1])  # correct way, works

axes_wrong = bh.axis.AxesTuple(axis1)  # wrong! But does not throw an error here

# THIS will error
axes_wrong.edges  # or any other attribute

[henryiii]

I'd be biased toward just throwing an error. Unless we supported AxesTuple(item1, item2, ...) syntax (which we don't, and neither does tuple), it just complicates the typing and could confuse users into thinking that would work. I don't think this is constructed by normal users much (at all?), so I think it's fine to be simple and strict. If for some reason this was really needed, we could always remove the error and make it work, but we can't go back the other way and make something that we add support for an error as easily.

Final call is @HDembinski's, though.

[jonas-eschle]

I'd be biased toward just throwing an error.

Sure, seems also reasonable to me

Unless we supported AxesTuple(item1, item2, ...) syntax (which we don't, and neither does tuple)

I didn't think about it but this seems actually intriguing to me as it aligns with the rest of the API such as the Histogram init.

But oc fine with anything, an error seems the least altering behavior.

[henryiii]

This is not really public IMO, at least it's not something a user should really be making. If it was something users would be expected to make regularly, it would be slightly more tempting. At the moment, it's more important that it matches the default tuple so code can support either one.

[HDembinski]

I agree with Henry that we should not add convenience syntax for classes that normal people should not be using, and the init of AxesTuple should precisely follow the init of tuple.

[HDembinski]

This is not really public IMO

It is not marked with _, so it is public by style convention.

[jonas-eschle]

I agree with Henry that we should not add convenience syntax for classes that normal people should not be using, and the init of AxesTuple should precisely follow the init of tuple.

Okay, I think @henryiii would go for an error, right? But makes sense, I think it's worth to add the error as the object won't function properly if a single axis is given.

Or if you prefer to keep it as is without an error, feel free to close

[henryiii]

Yes, I'd add an error if an AxesTuple is not make up of Axes, that part I agreed with. Just not adding convenience syntax.

It is not marked with _, so it is public by style convention

I meant the constructor, not the class; the class is public since it's a type returned from a public method, those always need to be public since they can be statically typed and interacted with - a user needs to be able to look up the methods, etc. Also, it's overridable by library implementations (notably Hist). I meant the constructor is not really public in that it's never documented as directly constructible by a user, and users never need to construct them in normal usage.