fix: Account for fuzzy edge values in rebinning with edges

[riga]

This PR fixes the rebinning of axes given a sequence of new edges that was recently added by #977.

Background

We recently discovered the new rebinning feature that only requires new edges values rather than bin count "groups", which is great! It requires that the requested edges coincide (that's where the problem is) with old ones so that bin content merging is logically unambiguous. However, we ran into an edge case where a single, new edge value is subject to float precision effects (e.g. 1.200000000000001).

The issue

Currently, this causes a bug in that the newly computed edges are missing an entry, resulting in axes dropping a bin. I extended the test_rebin_1d test case to use a fuzzy edge value, showing that the assertion in

boost-histogram/src/boost_histogram/tag.py

Lines 190 to 202 in fdfdae1

	assert all(
	np.isclose(
	axis.edges[np.abs(axis.edges - edge).argmin()],
	edge,
	)
	for edge in newedges
	), "Edges must be in the axis"
	matched_ixes = np.where(
	np.isin(
	axis.edges,
	newedges,
	)
	)[0]

passes due to np.isclose, but the subsequent index lookup via an exact np.isin fails: N edges were requested, but the rebinned axis only has N - 1.

Fix

I already prepared and tested a working fix,

matched_ixes = [np.abs(axis.edges - edge).argmin() for edge in newedges]
assert all(
    np.isclose(
        axis.edges[ix],
        edge,
    )
    for ix, edge in zip(matched_ixes, newedges)
), "Edges must be in the axis"

essentially storing the argmin results, doing the assertion, and then computing new edges with them.

If you agree, I can add the commit to this PR, but I thought it might be easier to run the test case and see that it indeed fails where it should/shouldn't.

[andrzejnovak]

Thanks for catching that @riga!

[henryiii]

The fix sounds good. Also, should this be an assert, or a raise AssertionError (or another Error)? Would we really want this check skipped if running without assertions?

[riga]

Good point. Imho, all of the checks should be mandatory. I'm changing this as well.

[andrzejnovak]

Could we emit the offending edge? It would make for easier debug by users. Something like

f"Edge(s) {missing_edges} not found in the axis. Nearest available edges are ..."

[riga]

Sure, just added that.

[henryiii]

I've found another (unrelated) bug, so likely will be making a patch release in the near future; I'll backport this too.

[riga]

I noticed that the test should actually compare against the true (exact) edges, which failed the tests. Should be fixed now.

[henryiii]

Thanks!