Skip to content

Commit

Permalink
Fix ROOT7 bin coordinate queries (and axis growth logic while I'm at …
Browse files Browse the repository at this point in the history 
…it) (#4688)

* Fix incorrect axis in RGetBinIndex axis growth logic

* Fix off-by-one in RGetBinIndex need-to-grow detection logic

* Make RFillBinCoord logic consistent with RGetBinIndex logic

* Add tests of GetBinFrom/GetBinCenter/GetBinTo
  • Loading branch information
@HadrienG2
HadrienG2 committed Dec 13, 2019
1 parent a06f3b4 commit fba31b1
Show file tree
Hide file tree
Showing 2 changed files with 156 additions and 15 deletions.
30 changes: 15 additions & 15 deletions hist/histv7/inc/ROOT/RHistImpl.hxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,8 @@ struct RGetBinIndex {
{
constexpr const int thisAxis = HISTIMPL::GetNDim() - I - 1;
int bin = std::get<thisAxis>(axes).FindBin(x[thisAxis]);
if (GROW && std::get<thisAxis>(axes).CanGrow() && (bin < 0 || bin > std::get<thisAxis>(axes).GetNBinsNoOver())) {
hist->GrowAxis(I, x[thisAxis]);
if (GROW && std::get<thisAxis>(axes).CanGrow() && (bin < 0 || bin >= std::get<thisAxis>(axes).GetNBinsNoOver())) {
hist->GrowAxis(thisAxis, x[thisAxis]);
status = RAxisBase::EFindStatus::kCanGrow;

// Abort bin calculation; we don't care. Let RHist::GetBinIndex() retry!
Expand Down Expand Up @@ -320,29 +320,29 @@ enum class EBinCoord {
kBinTo ///< Get the bin high edge
};

template <int I, class COORD, class AXES>
template <int I, int NDIM, class COORD, class AXES>
struct RFillBinCoord;

// Break recursion.
template <class COORD, class AXES>
struct RFillBinCoord<-1, COORD, AXES> {
template <int NDIM, class COORD, class AXES>
struct RFillBinCoord<-1, NDIM, COORD, AXES> {
void operator()(COORD & /*coord*/, const AXES & /*axes*/, EBinCoord /*kind*/, int /*binidx*/) const {}
};

/** Fill `coord` with low bin edge or center or high bin edge of all axes.
*/
template <int I, class COORD, class AXES>
template <int I, int NDIM, class COORD, class AXES>
struct RFillBinCoord {
void operator()(COORD &coord, const AXES &axes, EBinCoord kind, int binidx) const
{
int axisbin = binidx % std::get<I>(axes).GetNBins();
size_t coordidx = std::tuple_size<AXES>::value - I - 1;
constexpr const int thisAxis = NDIM - I - 1;
int axisbin = binidx % std::get<thisAxis>(axes).GetNBins();
switch (kind) {
case EBinCoord::kBinFrom: coord[coordidx] = std::get<I>(axes).GetBinFrom(axisbin); break;
case EBinCoord::kBinCenter: coord[coordidx] = std::get<I>(axes).GetBinCenter(axisbin); break;
case EBinCoord::kBinTo: coord[coordidx] = std::get<I>(axes).GetBinTo(axisbin); break;
case EBinCoord::kBinFrom: coord[thisAxis] = std::get<thisAxis>(axes).GetBinFrom(axisbin); break;
case EBinCoord::kBinCenter: coord[thisAxis] = std::get<thisAxis>(axes).GetBinCenter(axisbin); break;
case EBinCoord::kBinTo: coord[thisAxis] = std::get<thisAxis>(axes).GetBinTo(axisbin); break;
}
RFillBinCoord<I - 1, COORD, AXES>()(coord, axes, kind, binidx / std::get<I>(axes).GetNBins());
RFillBinCoord<I - 1, NDIM, COORD, AXES>()(coord, axes, kind, binidx / std::get<thisAxis>(axes).GetNBins());
}
};

Expand Down Expand Up @@ -446,7 +446,7 @@ public:
/// Get the center coordinate of the bin.
CoordArray_t GetBinCenter(int binidx) const final
{
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, CoordArray_t, decltype(fAxes)>;
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, DATA::GetNDim(), CoordArray_t, decltype(fAxes)>;
CoordArray_t coord;
RFillBinCoord()(coord, fAxes, Internal::EBinCoord::kBinCenter, binidx);
return coord;
Expand All @@ -455,7 +455,7 @@ public:
/// Get the coordinate of the low limit of the bin.
CoordArray_t GetBinFrom(int binidx) const final
{
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, CoordArray_t, decltype(fAxes)>;
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, DATA::GetNDim(), CoordArray_t, decltype(fAxes)>;
CoordArray_t coord;
RFillBinCoord()(coord, fAxes, Internal::EBinCoord::kBinFrom, binidx);
return coord;
Expand All @@ -464,7 +464,7 @@ public:
/// Get the coordinate of the high limit of the bin.
CoordArray_t GetBinTo(int binidx) const final
{
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, CoordArray_t, decltype(fAxes)>;
using RFillBinCoord = Internal::RFillBinCoord<DATA::GetNDim() - 1, DATA::GetNDim(), CoordArray_t, decltype(fAxes)>;
CoordArray_t coord;
RFillBinCoord()(coord, fAxes, Internal::EBinCoord::kBinTo, binidx);
return coord;
Expand Down
141 changes: 141 additions & 0 deletions hist/histv7/test/binning.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,18 @@ TEST(HistImplBinning, EquiDist2D) {
RAxisEquidistant, RAxisEquidistant>
hist(RAxisEquidistant(2, 0., 2.), RAxisEquidistant(2, -1., 1.));

// Here's a visual overview of how binning should work
//
// Axis 0
// UF 0.0 1.0 OF
// ------------------------
// A UF | 0 1 2 3
// x -1.0 | 4 5 6 7
// . 0.0 | 8 9 10 11
// 1 OF | 12 13 14 15

// Check that coordinates map into the correct bins

EXPECT_EQ( 0, hist.GetBinIndex({-100., -100.}));
EXPECT_EQ( 1, hist.GetBinIndex({0.5, -100.}));
EXPECT_EQ( 2, hist.GetBinIndex({1.5, -100.}));
Expand All @@ -137,4 +149,133 @@ TEST(HistImplBinning, EquiDist2D) {
EXPECT_EQ( 3, hist.GetBinIndex({ std::numeric_limits<double>::max(), -std::numeric_limits<double>::max()}));
EXPECT_EQ(12, hist.GetBinIndex({-std::numeric_limits<double>::max(), std::numeric_limits<double>::max()}));
EXPECT_EQ(15, hist.GetBinIndex({ std::numeric_limits<double>::max(), std::numeric_limits<double>::max()}));

// Check that bins map into the correct coordinates

const double uf_from = -std::numeric_limits<double>::max();
const double uf_center_axis0 = (uf_from + 0.0) / 2.0;
const double uf_center_axis1 = (uf_from - 1.0) / 2.0;
const double of_to = std::numeric_limits<double>::max();
const double of_center_axis0 = (2.0 + of_to) / 2.0;
const double of_center_axis1 = (1.0 + of_to) / 2.0;

// ... first bin on axis 1 ...

EXPECT_LE(uf_from, hist.GetBinFrom(0)[0]);
EXPECT_LE(uf_center_axis0, hist.GetBinCenter(0)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(0)[0]);
EXPECT_LE(uf_from, hist.GetBinFrom(0)[1]);
EXPECT_LE(uf_center_axis1, hist.GetBinCenter(0)[1]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinTo(0)[1]);

EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(1)[0]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(1)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(1)[0]);
EXPECT_LE(uf_from, hist.GetBinFrom(1)[1]);
EXPECT_LE(uf_center_axis1, hist.GetBinCenter(1)[1]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinTo(1)[1]);

EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(2)[0]);
EXPECT_FLOAT_EQ( 1.5, hist.GetBinCenter(2)[0]);
EXPECT_FLOAT_EQ( 2.0, hist.GetBinTo(2)[0]);
EXPECT_LE(uf_from, hist.GetBinFrom(2)[1]);
EXPECT_LE(uf_center_axis1, hist.GetBinCenter(2)[1]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinTo(2)[1]);

EXPECT_FLOAT_EQ( 2.0, hist.GetBinFrom(3)[0]);
EXPECT_GE(of_center_axis0, hist.GetBinCenter(3)[0]);
EXPECT_GE(of_to, hist.GetBinTo(3)[0]);
EXPECT_LE(uf_from, hist.GetBinFrom(3)[1]);
EXPECT_LE(uf_center_axis1, hist.GetBinCenter(3)[1]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinTo(3)[1]);

// ... next bin on axis 1 ...

EXPECT_LE(uf_from, hist.GetBinFrom(4)[0]);
EXPECT_LE(uf_center_axis0, hist.GetBinCenter(4)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(4)[0]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinFrom(4)[1]);
EXPECT_FLOAT_EQ(-0.5, hist.GetBinCenter(4)[1]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(4)[1]);

EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(5)[0]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(5)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(5)[0]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinFrom(5)[1]);
EXPECT_FLOAT_EQ(-0.5, hist.GetBinCenter(5)[1]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(5)[1]);

EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(6)[0]);
EXPECT_FLOAT_EQ( 1.5, hist.GetBinCenter(6)[0]);
EXPECT_FLOAT_EQ( 2.0, hist.GetBinTo(6)[0]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinFrom(6)[1]);
EXPECT_FLOAT_EQ(-0.5, hist.GetBinCenter(6)[1]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(6)[1]);

EXPECT_FLOAT_EQ( 2.0, hist.GetBinFrom(7)[0]);
EXPECT_GE(of_center_axis0, hist.GetBinCenter(7)[0]);
EXPECT_GE(of_to, hist.GetBinTo(7)[0]);
EXPECT_FLOAT_EQ(-1.0, hist.GetBinFrom(7)[1]);
EXPECT_FLOAT_EQ(-0.5, hist.GetBinCenter(7)[1]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(7)[1]);

// ... next bin on axis 1 ...

EXPECT_LE(uf_from, hist.GetBinFrom(8)[0]);
EXPECT_LE(uf_center_axis0, hist.GetBinCenter(8)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(8)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(8)[1]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(8)[1]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(8)[1]);

EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(9)[0]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(9)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(9)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(9)[1]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(9)[1]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(9)[1]);

EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(10)[0]);
EXPECT_FLOAT_EQ( 1.5, hist.GetBinCenter(10)[0]);
EXPECT_FLOAT_EQ( 2.0, hist.GetBinTo(10)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(10)[1]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(10)[1]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(10)[1]);

EXPECT_FLOAT_EQ( 2.0, hist.GetBinFrom(11)[0]);
EXPECT_GE(of_center_axis0, hist.GetBinCenter(11)[0]);
EXPECT_GE(of_to, hist.GetBinTo(11)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(11)[1]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(11)[1]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(11)[1]);

// ... last bin on axis 1 ...

EXPECT_LE(uf_from, hist.GetBinFrom(12)[0]);
EXPECT_LE(uf_center_axis0, hist.GetBinCenter(12)[0]);
EXPECT_FLOAT_EQ( 0.0, hist.GetBinTo(12)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(12)[1]);
EXPECT_GE(of_center_axis1, hist.GetBinCenter(12)[1]);
EXPECT_GE(of_to, hist.GetBinTo(12)[1]);

EXPECT_FLOAT_EQ( 0.0, hist.GetBinFrom(13)[0]);
EXPECT_FLOAT_EQ( 0.5, hist.GetBinCenter(13)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinTo(13)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(13)[1]);
EXPECT_GE(of_center_axis1, hist.GetBinCenter(13)[1]);
EXPECT_GE(of_to, hist.GetBinTo(13)[1]);

EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(14)[0]);
EXPECT_FLOAT_EQ( 1.5, hist.GetBinCenter(14)[0]);
EXPECT_FLOAT_EQ( 2.0, hist.GetBinTo(14)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(14)[1]);
EXPECT_GE(of_center_axis1, hist.GetBinCenter(14)[1]);
EXPECT_GE(of_to, hist.GetBinTo(14)[1]);

EXPECT_FLOAT_EQ( 2.0, hist.GetBinFrom(15)[0]);
EXPECT_GE(of_center_axis0, hist.GetBinCenter(15)[0]);
EXPECT_GE(of_to, hist.GetBinTo(15)[0]);
EXPECT_FLOAT_EQ( 1.0, hist.GetBinFrom(15)[1]);
EXPECT_GE(of_center_axis1, hist.GetBinCenter(15)[1]);
EXPECT_GE(of_to, hist.GetBinTo(15)[1]);
}

0 comments on commit fba31b1

Please sign in to comment.