Skip to content

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Mar 2, 2021

This is a much simpler to use family system. As a reminder, the old method (developed in #200, and which @HDembinski was still rightfully a little unhappy about in #456) is shown here:

CUSTOM_FAMILY = object()

@bh.utils.set_family(CUSTOM_FAMILY)
class Hist(bh.Histogram):
    ...

This requires that you know about the magic decorator when you subclass Histogram, and if you forget it, there's no warning, you just don't convert to the right Python class from C++ all the time.

Now that we are Python 3.6+, we can support a much better system! Here it is:

import hist

class Hist(bh.Histogram, family=hist):
    ...

There are two key changes. First, by making this a class keyword, this is now a missing "family" error if you do not set a family when subclassing! And, second, we change the recommendation for the tag object to the owning module. Any object supporting "is" still works, but this is a nice suggestion that also happens to be readable (boost_histogram is the family for the built-in classes). (Speaking of suggestions, not sure it's in the docs anywhere yet. I plan to do a docs sprint before 1.0). So users are now helped in making good subclasses, and don't have to find some weird bh.utils thing.

I'll take care up updating Hist with this; AFAIK that's the only user subclassing boost-histograms at the moment.

@github-actions github-actions bot added the needs changelog Might need a changelog entry label Mar 2, 2021
@henryiii henryiii added this to the 1.0.0 milestone Mar 2, 2021
@HDembinski
Copy link
Member

I don't really understand the details, but I trust you on this. Let me know if you want some real feedback, but I don't have any objections in case you want to move forward quickly.

@henryiii
Copy link
Member Author

henryiii commented Mar 3, 2021

Okay, I'll add to the changelog, and then merge. Once it's in master, I'll try it out downstream in Hist and make sure everything works. Since we have a "Hist-like" test now, I'm pretty sure it will be smooth.

@henryiii henryiii enabled auto-merge (squash) March 3, 2021 15:42
@github-actions github-actions bot removed the needs changelog Might need a changelog entry label Mar 3, 2021
@henryiii henryiii merged commit 8fcee7b into develop Mar 3, 2021
@henryiii henryiii deleted the refactor/family branch March 3, 2021 15:51
@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

This works in Hist correctly FYI, have an initial patch ready building against develop. :)

@alexander-held
Copy link
Member

Hi, cabinetry currently uses a class that inherits from bh.Histogram, and the nightly CI produced the "family" error described above. I could not find more information in the docs when searching for information about what this family system does, is it described somewhere?

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

I am planning to add it to the docs soon (before release), but no. You can use any unique token for the family. If you customize Axes too, then you also have to put the same whatever-it-is as you did for Histograms, and boost-histogram will know they are linked - you will always get your Axes (or Storages, or Transforms) from your Histograms.

The recommendation would be:

import cabinetry

class Histogram(bh.Histogram, family=cabinetry):

Now, if cabinetry only customizes Histogram, and not Axes, etc., then that's the one special case where family is not needed. I could make family optional on Histogram, but required on Axes, etc. But if you implement it on Axis, it has to match on Histogram.

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

An example of where it's used: h.project(0) makes a new histogram with a Regular axis. When that histogram converts axis 0 from C++, it needs to know which wrapper to use - the default one, or one that is further specialized (Hist, cabinetry.Histogram, etc). It looks the current Histogram._family, and compares it will all known subclasses of Regular - the one with the matching _family is used.

The same trick is used for converting (Hist(boost_hist_obj), etc), as well as loading pickles; anytime the C++ objects are turned into Python wrapped ones.

@alexander-held
Copy link
Member

Thanks! Only Histogram is customized, and I think that is unlikely to change. A naive question: how would I import the library (like import cabinetry in your example) from within the library, does that not lead to circular imports?

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

No, the first thing imported is always the main library, no matter what you do. Import a.b imports a first, even if everything inside b.py is relative.

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

If you don't ever need anything else, then you could just do family=object() and be done with it. :P

@alexander-held
Copy link
Member

Thank you! I will take a look at the changes to hist once they are propagated and read through the PRs linked above to learn more about this. At the moment family=object() seems to me like it might be enough.

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

Another idea would be to drop family from Histogram, and add histogram=Histogram to all "children" (Axis and such). The reason I didn't do this initially was it could be tricky with circular imports - Histogram imports Axes, but Axis would need Histogram to be defined. I think, as long as boost_histogram.Histogram was used instead of from boost_histogram import Histogram (and likewise in libraries), it might be okay, but that's why I didn't do it this way. I could try it out. It would avoid an extra concept (families), it would be a little less flexible (one master base Histogram required instead of "family", but this is a standard OO concept), and it would be much easier of users don't need custom Axes and such (which is likely the most common case seeing yours).

@henryiii
Copy link
Member Author

henryiii commented Mar 4, 2021

Just tried it, doesn't work due to circular imports. Working them out would be a bit of a pain.

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.

3 participants