Skip to content

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Sep 24, 2020

Adding an involved test that looks "Hist"-like, to make sure functionality Hist relies on is tested, like the ability to override AxesTuple with a subclass. Closes #430, as @HDembinski doesn't want name lookup in boost-histogram, at least for the foreseeable future.

Due to the design, adding custom functionality like a read only name is actually quite easy - subclasses just need to access ._ax.metadata directly. Didn't need a change, just a test.

Adds a simple hook that allows customizations of the conversion process by subclasses.

Adds ax.axes.<anything> as a proxy for ax.<anything>, making "metadata" no longer special here either.

@henryiii henryiii marked this pull request as draft September 24, 2020 02:54
@henryiii henryiii force-pushed the feat/casting branch 2 times, most recently from 2a83def to 7527191 Compare September 24, 2020 03:28
@henryiii henryiii marked this pull request as ready for review September 24, 2020 03:34
@henryiii henryiii merged commit f05bb5e into develop Sep 24, 2020
@henryiii henryiii deleted the feat/casting branch September 24, 2020 03:50
@HDembinski
Copy link
Member

The reason to drop the cpp interface was to simplify the code and make the extra wrapping and indirection obsolete. I don't see that happening here, as you continue to rely on that the set_family feature.

@HDembinski
Copy link
Member

Other people should wrap boost-histogram by subclassing or via composition.

@henryiii
Copy link
Member Author

henryiii commented Sep 24, 2020

Dropping CPP did simplify quite a bit, and removed the Base* classes, etc. (see #402). It was never supposed to or able to remove casting / set_family, since:

oax = CustomIntCategory(growth=True)
h = CustomHist(oax)
h.fill([1,2,3])
(ax,) = h.axes

What is ax? It's not even the same object as oax. Boost-histogram leaves this completely up to Boost.Histogram, and "casts" the cpp instance to Python when it is needed; when it does that, it has to know what to cast parts to. If Boost.Histogram adds any features to remove flow bins or change axes types, this system will be forward compatible with that. The other useful feature is that you can convert between histogram libraries naturally (Hist(th1.to_bh()) for example), and all axes get generated in the new family instead of the old one; if this didn't exist, you'd have to create and apply a mapping of boost-histogram <-> custom classes.

That is a way to replace this: you could track through all types through all boost-histogram operations and manually map types; you would also have to keep the type objects stored in the hist object (this also is needed for all other boost-histogram classes, like transforms and storages).

At the moment, that replacement would a very significant change, and could possibly be more error prone (at least would be more manual code / dict building) for a user library to set up than a simple decorator. A nice documentation page on how to do this might be better than trying to replace the system with another system with more manual intervention.

Also, this is subclassing; the family feature just enables consistent return types. If you only change Histogram, you don't need to add families; it's only for subclasses Axes, etc. (Since everyone will likely want to add names, I expect everyone will need to subclass at least Axis, though). You can't use composition because the underlying parts are in C++; templated C++ at that, so users cannot actually add a new Axes or subclass the C++ axes in C++; it wouldn't make a valid histogram<...>.

@HDembinski
Copy link
Member

HDembinski commented Sep 28, 2020

Ok, I think I understand, but this bothers me a bit. Perhaps it could be made so that the C++ code actually knows whether you pass a subclassed Axis to the histogram. We can already store arbitrary metadata in the metadata field, so in principle we could also store the subclassing info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

name/title system

2 participants