From e4426e4633e481c69d120370cf424271956933e1 Mon Sep 17 00:00:00 2001 From: Matthias Kleiner Date: Sat, 14 Nov 2026 11:31:01 +0100 Subject: [PATCH] TPC: fix pair swap when reading CalArray written by the custom streamer The custom member streamer for CalArray::mData writes the count N/2 followed by N single shorts, but reads N/2 ints and reinterprets each as a pair of flags (the layout written by ROOT < 6.33). Every object written by the streamer itself (ROOT >= 6.33) therefore comes back with each pair of adjacent pads swapped. This affects all IDC_PadStatusMap objects written since the streamer was introduced (e.g. 2026 data, ROOT 6.36.04): the dead channel map used in reconstruction marks the neighbour of an isolated bad pad instead of the pad itself. Objects written with ROOT < 6.33 (e.g. 2025, ROOT 6.32.06) are read correctly. The class version did not change, so the reader now selects the layout by the ROOT version that wrote the buffer (TBuffer::GetVersionOwner(); the current ROOT if the buffer has no file): < 6.33 as before, >= 6.33 N shorts. The writer is unchanged, so existing objects stay readable and no re-upload is needed. Add a ROOT I/O round-trip unit test for CalDet, which fails with the old reader (349440 of 524160 flags differ) and passes with the new one. Co-Authored-By: Claude Opus 5.5 --- .../src/TPCFlagsMemberCustomStreamer.cxx | 36 +++++++++++++++--- .../TPC/baserecsim/test/testTPCCalDet.cxx | 38 +++++++++++++++++++ 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/Detectors/TPC/baserecsim/src/TPCFlagsMemberCustomStreamer.cxx b/Detectors/TPC/baserecsim/src/TPCFlagsMemberCustomStreamer.cxx index 27ebfeb3c64bb..d7fd9bf48444b 100644 --- a/Detectors/TPC/baserecsim/src/TPCFlagsMemberCustomStreamer.cxx +++ b/Detectors/TPC/baserecsim/src/TPCFlagsMemberCustomStreamer.cxx @@ -12,6 +12,7 @@ #include "TPCBase/CalArray.h" #include #include +#include #include #include @@ -26,6 +27,14 @@ // This member was written incorrectly to the TFile in ROOT versions < 6.36, causing // segfaults when reading on ARM64 (occassionally). // We continue to write it in the incorrect format and fix the reading back. +// +// Two layouts exist on disk, both with the count N/2 (N = number of flags): +// - written by ROOT < 6.33 (no custom streamer): N/2 ints, each holding a pair of +// flags as they sit in memory; +// - written by this streamer (ROOT >= 6.33): N single shorts. +// Reading the second layout as ints swaps every pair of adjacent flags, so the +// layout is chosen by the ROOT version that wrote the buffer. The class version +// did not change, so this is the only marker. // See also: // - https://github.com/root-project/root/pull/17009 @@ -40,10 +49,27 @@ void MemberVectorPadFlagsStreamer(TBuffer& R__b, void* objp, int n) std::vector* obj = static_cast*>(objp); if (R__b.IsReading()) { obj->clear(); - std::vector R__stl; - R__stl.clear(); int R__n; R__b >> R__n; + // ROOT version of the file the buffer belongs to (e.g. 63206); 0 without a file (e.g. TMessage): + // then writer and reader run the same ROOT + int writerVersion = R__b.GetVersionOwner(); + if (writerVersion <= 0) { + writerVersion = gROOT->GetVersionInt(); + } + constexpr size_t bloatfactor = sizeof(int) / sizeof(o2::tpc::PadFlags); + if (writerVersion >= 63300) { + // written by this streamer: N single shorts + obj->reserve(bloatfactor * R__n); + for (size_t i = 0; i < bloatfactor * R__n; ++i) { + Short_t readtemp; + R__b >> readtemp; + obj->push_back(static_cast(static_cast(readtemp))); + } + return; + } + // written by ROOT < 6.33: N/2 ints, each a pair of flags in memory order + std::vector R__stl; R__stl.reserve(R__n); for (int R__i = 0; R__i < R__n; R__i++) { Int_t readtemp; @@ -51,12 +77,12 @@ void MemberVectorPadFlagsStreamer(TBuffer& R__b, void* objp, int n) R__stl.push_back(readtemp); } auto data = reinterpret_cast(R__stl.data()); - constexpr size_t bloatfactor = sizeof(int) / sizeof(o2::tpc::PadFlags); - for (int i = 0; i < bloatfactor * R__n; ++i) { + for (size_t i = 0; i < bloatfactor * R__n; ++i) { obj->push_back(static_cast(data[i])); } } else { - // We always save things with the old format. + // count N/2, then N single shorts: NOT the old (ROOT < 6.33) layout of N/2 ints; the reader above + // tells the two apart by the ROOT version. Unchanged, so that existing objects stay readable. R__b << (int)obj->size() / 2; for (size_t i = 0; i < obj->size(); i++) { R__b << (short)obj->at(i); diff --git a/Detectors/TPC/baserecsim/test/testTPCCalDet.cxx b/Detectors/TPC/baserecsim/test/testTPCCalDet.cxx index d9582dd334638..4ee0e7feacbc9 100644 --- a/Detectors/TPC/baserecsim/test/testTPCCalDet.cxx +++ b/Detectors/TPC/baserecsim/test/testTPCCalDet.cxx @@ -346,6 +346,44 @@ BOOST_AUTO_TEST_CASE(CalDetTypeTest) BOOST_CHECK(testDict == true); } +BOOST_AUTO_TEST_CASE(CalDetPadFlags_ROOTIO) +{ + // CalArray::mData has a custom member streamer (TPCFlagsMemberCustomStreamer.cxx); + // a write + read round trip must give back every flag at its own pad (a pattern in which + // adjacent pads always differ, so that a swap of neighbouring pads shows up) + CalDet flags("flags", PadSubset::Region); + const PadFlags bad = PadFlags::flagSkip | PadFlags::flagLowPad; + size_t n = 0; + for (auto& arr : flags.getData()) { + for (size_t i = 0; i < arr.getData().size(); ++i, ++n) { + arr.setValue(i, (n % 3 == 0) ? bad : PadFlags::flagGoodPad); + } + } + + auto f = TFile::Open("CalDetPadFlags_ROOTIO.root", "recreate"); + f->WriteObject(&flags, "flags"); + f->Close(); + delete f; + + f = TFile::Open("CalDetPadFlags_ROOTIO.root"); + auto* read = f->Get>("flags"); + BOOST_REQUIRE(read != nullptr); + BOOST_REQUIRE_EQUAL(read->getData().size(), flags.getData().size()); + size_t nDiffer = 0; + for (size_t iarr = 0; iarr < flags.getData().size(); ++iarr) { + const auto& written = flags.getCalArray(iarr).getData(); + const auto& back = read->getCalArray(iarr).getData(); + BOOST_REQUIRE_EQUAL(back.size(), written.size()); + for (size_t i = 0; i < written.size(); ++i) { + nDiffer += back[i] != written[i]; + } + } + BOOST_CHECK_EQUAL(nDiffer, 0); + delete read; + f->Close(); + delete f; +} + BOOST_AUTO_TEST_CASE(CalDetStreamerTest) { // simple code executing the TPC IDCPadFlags loading in a standalone env --> easy to valgrind