Use numpy histogram_bin_edges to define bins

[dougiesquire]

Description

Currently bins must be a list of arrays despite what the documentation says. This PR makes it so that users can provide bins in broadly the same way as they do for np.histogram (i.e. as an int, str, or array).

Closes #13

Type of change

• [x] Bug fix (non-breaking change which fixes an issue)

Testing

• [x] Added multiple tests to test_core.py.

Things to note

• Parsing bins is a little fiddly in xhistogram because we must allow for lists of bins - one for each input array. For example, if a user were to specify bins=[1,2,3] for 3 args, how do we know whether they want:

  • bin edges [1,2,3] applied to all 3 args; or
  • arg_1 use 1 bin, arg_2 use 2 bins, arg_3 use 3 bins?

  My current way of getting around this is to require that when users want bin edges they specify them as a numpy array

• To align with the np.histogram API and its definition of bins, I've added an additional parameter, range, to the xhistogram APIs.

• This PR also required that I change the outputs of .core.histogram to return both the histogram and the bins. This may constitute a breaking change for people using the core module.

[codecov[bot]]

Codecov Report

Merging #45 (d5a23c2) into master (c62cf9f) will increase coverage by 1.19%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   94.54%   95.74%   +1.19%     
==========================================
  Files           3        3              
  Lines         220      235      +15     
  Branches       58       67       +9     
==========================================
+ Hits          208      225      +17     
+ Misses          8        7       -1     
+ Partials        4        3       -1     

Impacted Files	Coverage Δ
xhistogram/core.py	100.00% <100.00%> (+1.48%)	:arrow_up:
xhistogram/xarray.py	91.07% <100.00%> (-0.87%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c62cf9f...d5a23c2. Read the comment docs.

[rabernat]

Thanks a lot for your PR @dougiesquire! I hope to review it within a few days. Thanks for your patience.

[rabernat]

So now that #50 is ready, should we merge this?

[dougiesquire]

So now that #50 is ready, should we merge this?

This comment makes me realise I misunderstand the process here. I had thought we were going to hold off on merging this and merge/release #50 so that users will get the warning about this upcoming change before it is merged. Can you please set me straight?

[rabernat]

No I think I'm the one who misunderstood! 🙃 It totally makes sense that we merge #50 first. But then we should do a release right away (v0.2), with #50, and then make this part of the next release (v0.3). This involves a lot of releases for a project that doesn't release very often. But I think it's the right way to go.

Just to be sure we are on the same page, let me describe my understanding of the deprecation cycle.

Releases

All the matters to users is releases. Our latest release, available on pip and conda, is v0.1.3. We don't have to assume that users are installing from the latest github master branch. Most of the dev work we are doing now is ostensibly going to be part of the next release, probably v0.2, given the significant feature changes. (We use semantic versioning, like most python packages.) It's okay if the master branch is temporarily in a slightly inconsistent or even broken state, as long as everything is cleaned up by the next release. So in this view, the order of operations (this vs #50) doesn't matter; they will both be merged by the next release.

Deprecation

Deprecation cycles give our users, especially anyone using xhistogram in a "production" context, to get warnings of upcoming API changes before they become defaults / breaking changes. If we merge #50 and the do our next release (v0.2), we will start raising these FutureWarnings about the core.histogram return stuff. We would then merge this after that and include it as part of the next release (v0.3).

[dougiesquire]

Yup - that aligns with my thinking. I'll go ahead and merge #50 and then have a go at a release? (this will be my first release so apologies in advance if I have questions)

[rabernat]

Beat you merging #50. Full speed ahead with the release. The CI should be set up so you can just make a release on GitHub. No other manual steps required.

[rabernat]

Ok great, now that 0.2 is out, we can merge this (once you have resolved the conflicts).

[dougiesquire]

I've also removed the FutureWarning here. I think this is the right thing to do since we don't want to keep warning the user that a change is coming after the change has been made. Let me know if I've made the wrong call here and I'll add it back in.