Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support _to_boost_ #483

Merged
merged 4 commits into from
Feb 18, 2021
Merged

feat: support _to_boost_ #483

merged 4 commits into from
Feb 18, 2021

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Dec 18, 2020

Pulled the change out from #475 so that it can get a proper discussion if it needs it.

Boost-histogram already supports casting - you can call Hist(a_boost_object), where Hist is any boost-powered project, including Hist, and it converts. Likewise with Histogram(a_hist_object) . This allows uproot to participate - you can call bh.Histogram(an_uproot_histogram) and it converts by first calling to_boost, then using the standard mechanism to convert it to bh.Histogram (or Hist, or any other dependent project if more get added).

The main question I'd like to discuss is: should we use the current method uproot provides, .to_boost(), or instead provide a hidden method to do this, like _to_boost_(), and add it to uproot? If Physt were to add a method, I bet Jan would be fine with to_boost(), but not requiring a user-visible method might be better? Also, in the future, we might want to provide a way to produce a structure that boost-histogram can adapt to reduce coupling - but that's very far away - sticking to a hidden method now would keep us from having to expose a public method tied to the old method if a new one was added.

Edit: As seen below, I went with _as_boost_histogram_().

@github-actions github-actions bot added the needs changelog Might need a changelog entry label Dec 18, 2020
@henryiii
Copy link
Member Author

Tests are failing because of the new setup-python release: actions/setup-python#171. We can pin to an older one until it's fixed. (Though the new release is really exciting, it now supports all versions of PyPy, in all flavors! Well, it would, anyway, if it worked...) :)

@henryiii
Copy link
Member Author

(actually intended to write this on another PR, but guess it's okay here.)

@henryiii
Copy link
Member Author

I'm bothered by the fact this is a public method. We might come up with a better method design later, so how about we select _as_boot_histogram_, and that makes it easier to replace later if we have a better method without littering up downstream project's namespaces if they want to avoid adding a public method. Uproot histogram could simply do _as_boost_histogram_ = to_boost and start supporting this.

@henryiii
Copy link
Member Author

henryiii commented Feb 9, 2021

@jpivarski would you be okay if we make this _as_boost_histogram, and uproot gets a _as_boost_histogram_ = to_boost line?

@henryiii henryiii modified the milestones: 1.1.0, 1.0.0 Feb 9, 2021
@jpivarski
Copy link
Member

jpivarski commented Feb 9, 2021

@jpivarski would you be okay if we make this _as_boost_histogram_, and uproot gets a _as_boost_histogram_ = to_boost line?

Sure, if you do the pull request on Uproot, I'll accept it. You'll have to coordinate versions, but I can make an Uproot release candidate immediately after your PR.

@henryiii henryiii changed the title feat: support to_boost feat: support _as_boost_histogram_ Feb 9, 2021
@henryiii henryiii modified the milestones: 1.0.0, 0.13.0 Feb 9, 2021
@HDembinski
Copy link
Member

Hidden methods are better than public methods. The long name bothers me, why not call it simply _to_boost.

@henryiii
Copy link
Member Author

henryiii commented Feb 14, 2021

My thought was this will never have to be typed, and someone coming across a method called _to_boost_histogram_ would know exactly what library to look for, while _to_boost_ could be a bit confusing. But this will always be on something that can be viewed as a histogram, so happy with any name, really. to is probably better than as.

So, now that you know my reasoning, do you like _to_boost_histogram_ or _to_boost_ better? This serves the same function as __array__ does for NumPy.

@HDembinski
Copy link
Member

All those arguments are valid, but for consistency/symmetry with to_numpy etc., I would still prefer _to_boost.

@henryiii henryiii changed the title feat: support _as_boost_histogram_ feat: support _to_boost_ Feb 16, 2021
@henryiii
Copy link
Member Author

Okay, though matching to_<package_name> was my exactly my proposal _to_boost_histogram_. We don't own the boost package on PyPI (though might be worth asking if we could get it, it would be nice to make it a namespace package for the boost libraries :) )

@jpivarski
Copy link
Member

In following this, I've also been thinking that "_to_<package name>_" would be best because then there are no special cases, like shortening "boost_histogram" → "boost". The whole package name is pasted in as-is, and that makes it more consistent.

Since this is an underscored name, brevity is not important: users will not be typing it. However, it's part of a protocol for developers, so it is important what that name is, and the need for consistency is heightened. (After all, these underscores don't mean "private"—a name like __add__, __array__, or _ipython_key_completions_ has to be more stable than even a user-facing interface.)

@henryiii
Copy link
Member Author

henryiii commented Feb 16, 2021

Also, .to_numpy and ._to_boost_histogram_/_to_boost_ are totally different; we provide .to_numpy(), but we do not provide ._to_boost_histogram_ - we already are a boost histogram! Libraries that implement this protocol will not necessary have a to_numpy also. (Uproot4 does, of course).

@henryiii
Copy link
Member Author

@HDembinski please confirm one of the two proposed names _to_boost_histogram_ or _to_boost_ (keeping in mind this is technically added by downstream libraries, not us, and the author of the main downstream library in Scikit-HEP is in favor of the more descriptive name). We can release 0.13 after this goes in. I’d like to have it sit for a few weeks while I’m off. I’ve already missed several important deadlines waiting for a response from you on this issue and the other one, including the SciPy proposal, my biweekly work report, and the birth of my daughter. If you insist on complete creative control over a library I proposed, wrote 87% of, and get paid for, you could at least try to be responsive when I ask for input. I did a same day release of 0.12 when you asked for it for your analysis. If you don’t have time for boost-histogram, I would always be happy to regain creative control. ;)

PS: I finally merged the other PR, #498, since I make the requested change related to the PR, updated the explanation, and the unrelated change of dropping dd= should be opened as an issue if you want it (though I would rather leave it matching uproot).

@HDembinski
Copy link
Member

HDembinski commented Feb 18, 2021

I am sorry for letting you down on this and I apprecitate how fast you reacted on the release. I choose _to_boost_histogram_ then. I am overwhelmed with work, it is not that I intentionally ignore this.

Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
@HDembinski HDembinski merged commit ae0a229 into develop Feb 18, 2021
@HDembinski HDembinski deleted the feat/to_boost branch February 18, 2021 13:57
@henryiii
Copy link
Member Author

Thank you @HDembinski! Sorry I snapped a bit. Not getting much sleep here. 👶 I'm going to get a 0.13 release out so it can sit for a bit while I'm off before the deprecation/Py2 cleanup for 1.0 starting in March!

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.

None yet

3 participants