Skip to content

[treeplayer] Fix TTreeFormula vector index into vector-of-vectors #22601

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.

Sign up for GitHub

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

Open
guitargeek wants to merge 1 commit into
base: master
Choose a base branch
from
+119 −6
Open

[treeplayer] Fix TTreeFormula vector index into vector-of-vectors #22601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions tree/treeplayer/src/TTreeFormula.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,22 @@ void TTreeFormula::DefineDimensions(Int_t code, Int_t size,
Int_t& virt_dim) {
if (info) {
fManager->EnableMultiVarDims();
//if (fIndexes[code][info->fDim]<0) { // removed because the index might be out of bounds!
// When the primary (first) dimension is selected through a variable index
// (a "gather", fIndexes[code][0]==-2) and this inner variable-size dimension
// is itself pinned to a constant index, the formula does not iterate over the
// jagged sub-dimension: each gathered row yields a single value, so the number
// of instances is simply the length of the index variable. Registering the
// dimension as a variable one would make the manager sum the physical
// sub-sizes (e.g. vv1[v2][0] would loop over the total size of vv1 instead of
// the length of v2). So only register it when it actually varies during the
// iteration. See https://github.com/root-project/root/issues/19290
bool pinnedInnerDuringGather = (fIndexes[code][0] == -2) && (fIndexes[code][fNdimensions[code]] >= 0);

@pcanal pcanal Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smell a little too specific. Can we verify whether we get the correct behavior for:

vv1[v2][v3]

@guitargeek guitargeek Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it doesn't work, but what do you expect to come out of it? This would first need a decision about the semantics.

Would you expect v2 and v3 to get zipped together to form index tuples (vectorized indexing)?
Like this, in pseudo code:

vv1 = [[1, 2, 3],
           [4, 5, 6],
           [7, 8, 9]]

v2 = [0, 1]
v3 = [0, 1]

vv1[v2][v3] resolves to vv1[[0, 1], [0, 1]], becoming [1 5]

Or would you expect it to select slices (orthogonal indexing)?

vv1[v2][v3] resolves to [[1, 2],
                                     [4, 5]]

Is the expected behavior documented anywhere? If not, and it given that this doesn't work before and after this PR, I think a) the fact that nobody requested it and b) the semantic ambiguity leads me to the conclusion that this is a candidate for a known limitation, and we should just make sure there is a proper error.

But if you pick a semantic and it turns out to be easy to implement, we can also try to go for that.

By the way, NumPy uses the index tuple convention for its bracket operator:

import numpy as np

vv1 = np.array([[1, 2, 3], [4, 5, 6]])

v2 = np.array([0, 1])
v3 = np.array([0, 1])

print(vv1[v2, 0])
print(vv1[0, v3])
print(vv1[v2, v3])
[1 4]
[1 2]
[1 5]

So if I'd get to pick, I would go for this meaning.

if (!pinnedInnerDuringGather) {
// NOTE: a dimension with a (constant) index is still registered here, because the
// index might be out of bounds and the dimension must still be tracked.
info->fVirtDim = virt_dim;
fManager->AddVarDims(virt_dim); // if (!fVarDims[virt_dim]) fVarDims[virt_dim] = new TArrayI;
//}
}
}

Int_t vsize = 0;
Expand Down Expand Up @@ -3631,8 +3643,11 @@ Int_t TTreeFormula::GetRealInstance(Int_t instance, Int_t codeindex) {
} else {
local_index = instance;
}
if (info && local_index>=fCumulSizes[codeindex][max_dim]) {
if (info && fIndexes[codeindex][max_dim] != -2 && local_index >= fCumulSizes[codeindex][max_dim]) {
// We are out of bounds! [Multiple var dims, See same message a few line above]
// For a variable index (-2) local_index is the instance number of the
// index variable, not the physical position; the actual bounds are
// checked below after evaluating the index variable.
return fNdata[0]+1;
}
if (fIndexes[codeindex][max_dim]==-2) {
Expand Down
104 changes: 101 additions & 3 deletions tree/treeplayer/test/regressions.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
// ROOT-10702
TEST(TTreeReaderRegressions, CompositeTypeWithNameClash)
{
struct Int { int x; };
struct Int {
int x;
};
gInterpreter->Declare("struct Int { int x; };");

const auto fname = "ttreereader_compositetypewithnameclash.root";
Expand Down Expand Up @@ -196,7 +198,7 @@ TEST(TSelectorDrawRegressions, TernaryOperator)
t.Draw("(1?2:3)>>h1(12345,0,20)");
auto h = gROOT->Get<TH1>("h1");
ASSERT_EQ(h->GetXaxis()->GetNbins(), 12345); // was ignored before and set to the default 100
ASSERT_EQ(h->GetBinContent(1235), 1); // FindBin(2) is at 1235
ASSERT_EQ(h->GetBinContent(1235), 1); // FindBin(2) is at 1235
}

// ROOT-4012 (JIRA)
Expand All @@ -215,7 +217,10 @@ TEST(TTreeFormulaRegressions, ConstantAlias)
}

// ROOT-8577 (JIRA)
#define MYSTRUCT struct MyS { int x; };
#define MYSTRUCT \
struct MyS { \
int x; \
};
MYSTRUCT
TEST(TTreeFormulaRegressions, WrongName)
{
Expand Down Expand Up @@ -661,3 +666,96 @@ TEST(TTreeScan, ULong64Precision)
EXPECT_EQ(scanToString("colsize=21 col=lld:lld:lld"), expectedScanOut);
EXPECT_EQ(scanToString("col=21lld:21lld:21lld"), expectedScanOut);
}

// https://github.com/root-project/root/issues/19290 (originally JIRA ROOT-8726)
// TTreeFormula gave wrong results when a vector-type branch was used as the index
// of a vector-of-vectors branch. For vv1[v2][0] (v2 a vector used as the index of
// the outer dimension of the vector-of-vectors vv1) the number of instances was
// computed as the total/summed size of all the sub-vectors of vv1 instead of the
// length of v2, and out-of-bounds errors were emitted. The companion case
// vv1[0][v2] (constant outer index, vector inner index) silently dropped its last
// instance.
namespace {

// Evaluate a TTreeFormula on the (single) current entry of the tree and return
// all of its instances.
std::vector<double> GH19290EvalAll(TTree &tree, const char *expr)
{
TTreeFormula formula("formula", expr, &tree);
EXPECT_FALSE(formula.GetTree() == nullptr) << "could not compile formula " << expr;

std::vector<double> result;
const Int_t ndata = formula.GetNdata();
result.reserve(ndata);
for (Int_t i = 0; i < ndata; ++i)
result.push_back(formula.EvalInstance(i));
return result;
}

} // namespace

struct GH19290 : public ::testing::Test {
TTree fTree{"t", "t"};

int fN = 1;
// The branch addresses must outlive SetUp(), so keep the objects and the
// pointers used by TTree::Branch as data members.
std::vector<int> fV1{5, 7};
std::vector<std::vector<int>> fVV1{{2, 1}, {3, 4}};
std::vector<int> fV2{0, 1, 0};
std::vector<int> *fV1Ptr = &fV1;
std::vector<std::vector<int>> *fVV1Ptr = &fVV1;
std::vector<int> *fV2Ptr = &fV2;

void SetUp() override
{
gInterpreter->GenerateDictionary("vector<vector<int>>");

fTree.Branch("n", &fN);
fTree.Branch("v1", &fV1Ptr);
fTree.Branch("vv1", &fVV1Ptr);
fTree.Branch("v2", &fV2Ptr);
fTree.Fill();
fTree.GetEntry(0);
}
};

// Sanity: indexing a plain vector with a vector index already worked.
TEST_F(GH19290, VectorIndexIntoVector)
{
// v1[v2] loops over v2 == {0,1,0} -> {v1[0], v1[1], v1[0]}
EXPECT_EQ(GH19290EvalAll(fTree, "v1[v2]"), (std::vector<double>{5, 7, 5}));
}

// Constant outer index, vector inner index. Used to drop the last instance.
TEST_F(GH19290, ConstantOuterVectorInner)
{
// vv1[0][v2] loops over v2 -> {vv1[0][0], vv1[0][1], vv1[0][0]}
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[0][v2]"), (std::vector<double>{2, 1, 2}));
}

// Vector outer index, constant inner index. Used to over-count (length was the
// summed size of all sub-vectors) and emit out-of-bounds errors.
TEST_F(GH19290, VectorOuterConstantInner)
{
// vv1[v2][0] loops over v2 -> {vv1[0][0], vv1[1][0], vv1[0][0]}
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[v2][0]"), (std::vector<double>{2, 3, 2}));
}

// Scalar indices into a vector of vectors (these always worked).
TEST_F(GH19290, ScalarIndices)
{
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[n][0]"), (std::vector<double>{3})); // vv1[1][0]
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[0][n]"), (std::vector<double>{1})); // vv1[0][1]
}

// A few related expressions that must keep working (no regressions).
TEST_F(GH19290, NoRegressions)
{
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[0]"), (std::vector<double>{2, 1}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[1]"), (std::vector<double>{3, 4}));

@pcanal pcanal Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[1]"), (std::vector<double>{3, 4}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[1]"), (std::vector<double>{...}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[0][1]"), (std::vector<double>{...}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[1][1]"), (std::vector<double>{...}));

for good measures

@guitargeek guitargeek Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

EXPECT_EQ(GH19290EvalAll(fTree, "vv1[0][1]"), (std::vector<double>{1}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1[1][1]"), (std::vector<double>{4}));
EXPECT_EQ(GH19290EvalAll(fTree, "vv1"), (std::vector<double>{2, 1, 3, 4}));
EXPECT_EQ(GH19290EvalAll(fTree, "v1[n]"), (std::vector<double>{7}));
}
Loading