-
Notifications
You must be signed in to change notification settings - Fork 24
Add RNTuple support to QwCorrelator #191
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.
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
base: main
Are you sure you want to change the base?
Conversation
| // TODO this should also check for IsTreeDisabled(name) as in NewTree(), | ||
| // --disable-trees sets IsTreeDisabled for all possible trees, and that | ||
| // is required for RNTuples to be enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulmking Note comment. --disable-trees adds .* to the regex pattern we use. We probably want to have that same tree selection interface apply to rntuples. But that would means we turn --enable-rntuples into the option that implicitly disables writing to trees (since we can only do one or the other).
|
RFR @paulmking |
…Tuple - Add ROOT::RNTupleModel* GetModel() and ROOT::RNTupleWriter* GetWriter() to QwRootNTuple to expose the underlying model/writer (return raw pointers from the stored unique_ptrs). - Refine QwRootFile::NewNTuple: only early-return when RNTuples are disabled (fEnableRNTuples), log creation, and initialize the writer. (A TODO notes that IsTreeDisabled should also be enforced similar to NewTree.) - Add a HAS_RNTUPLE_SUPPORT-guarded QwRootFile::GetNTuple(name) helper to retrieve a registered RNTuple by name. These changes make RNTuple internals accessible where needed and add convenience retrieval and clearer creation logging.
…nt NTuple field construction & filling - Add HAS_RNTUPLE_SUPPORT guarded members to QwCorrelator (fNTupleModel, fNTupleWriter and vectors of shared_ptrs for int/long/vector/matrix fields and matrix dims). - Declare ConstructNTupleFields / FillNTupleFields in header under the RNTuple guard. - Implement ConstructNTupleFields in QwCorrelator.cc: create an RNTuple model/writer, register scalar, vector and matrix fields using MakeField, and add separate row/col fields for matrices. - Implement NTuple population in CalcCorrelations (guarded by HAS_RNTUPLE_SUPPORT): add helper lambdas to flatten TMatrixD/TVectorD into std::vector<Double_t>, populate scalar, vector and matrix fields, store matrix row/col counts, and use fNTupleWriter->Fill() when no TTree is used. - Preserve behavior when RNTuple support is not enabled (all additions are conditional on HAS_RNTUPLE_SUPPORT).
- Ensure QwDataHandlerArray exposes/caches the enable-rntuples option (fEnableRNTuples) - Short-circuit ConstructNTupleFields and FillNTupleFields when RNTuples are disabled (wrapped in HAS_RNTUPLE_SUPPORT) - In QwCorrelator, only prepare/copy RNTuple fields when fNTupleWriter exists (move flattening lambdas inside the check) - Call fNTupleWriter->Fill() separately (no longer mutually exclusive with TTree filling) This prevents wasted computation when RNTuples are disabled and makes NTuple writing conditional on an actual writer.
724c749 to
396fa89
Compare
| } | ||
|
|
||
| /// Get the model of the RNTuple | ||
| ROOT::RNTupleModel* GetModel() const { return fModel.get(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In creating a RNTupleWriter,
fWriter = ROOT::RNTupleWriter::Append(std::move(fModel), fName, *file);
leaves fModel as a nullptr.
Careless use of GetModel() will have us returning (and likely using) a nullptr. ROOT::RNTupleWriter does have a GetModel method
const ROOT::RNTupleModel& ROOT::RNTupleWriter::GetModel() const
If we try to wrap this method, the tricky part is then that fWriter might not be initialized when called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get "lucky" in our use inside QwCorrelator::ConstructNTupleFields(), because we return from QwRootNTuple::IntializeWriter without actually initializing the writer, so fModel is non-null
(I scare quote lucky because maybe that was the original intention..?)
| branchv(fNTupleModel,linReg.mSYp, "dMYp"); // Corrected mean error | ||
|
|
||
| // Create and get the writer for the model | ||
| fNTupleWriter = ntuple->GetWriter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How we have the code setup,
treerootfile->NewNTuple(name, fTreeComment.c_str()); calls
QwRootNTuple::IntializeWriter which returns without initializing the writer since fVector is emtpy at this point
if (fVector.empty()) {
QwError << "No fields defined in RNTuple model for " << fName << QwLog::endl;
return;
}
try { /* init writer */ ... }
I do not think we ever actually init the writer, so
fNTupleWriter = ntuple->GetWriter(); will be a nullptr
Using gdb shows this to be true:
1: fNTupleWriter = (ROOT::RNTupleWriter *) 0x0
(gdb) list
666 branchv(fNTupleModel,linReg.mSY, "dMY"); // Uncorrected mean error
667 branchv(fNTupleModel,linReg.mSYp, "dMYp"); // Corrected mean error
668
669 // Create and get the writer for the model
670 fNTupleWriter = ntuple->GetWriter();
671 }
672 #endif
673
674 /// \brief Construct the histograms in a folder with a prefix
675 void QwCorrelator::ConstructHistograms(TDirectory *folder, TString &prefix)
(gdb) p fNTupleWriter
$26 = (ROOT::RNTupleWriter *) 0x0
(gdb) where
#0 QwCorrelator::ConstructNTupleFields (this=0x55555b9d1560, treerootfile=<optimized out>, treeprefix=..., branchprefix=...) at /home/mrc/Github/PR191/Parity/src/QwCorrelator.cc:671
#1 0x00007ffff7cdfc76 in QwDataHandlerArray::ConstructNTupleFields (this=this@entry=0x7fffffff7b00, treerootfile=treerootfile@entry=0x55555d6968f0, treeprefix="burst_", branchprefix="|stat")
at /home/mrc/Github/PR191/Parity/src/QwDataHandlerArray.cc:384
#2 0x0000555555564bfd in main (argc=<optimized out>, argv=<optimized out>) at /home/mrc/Github/PR191/Parity/main/QwParity.cc:244
|
|
||
| #ifdef HAS_RNTUPLE_SUPPORT | ||
| // Fill RNTuple field pointers | ||
| if (fNTupleWriter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fNTupleWriter will always be null here and so we will never write to file.
See comment regarding QwCorrelator::ConstructNTupleFields()
(gdb) p fNTupleWriter
$4 = (ROOT::RNTupleWriter *) 0x0
(gdb) list
236 linReg.printSummaryMeansWithUncCorrected();
237 }
238 }
239
240 #ifdef HAS_RNTUPLE_SUPPORT
241 // Fill RNTuple field pointers
242 if (fNTupleWriter) {
243 // Helper lambdas to flatten matrix and vector to std::vector
244 // Note: these rely on RVO with copy elision to avoid unnecessary copies
245 auto flattenMatrix = [](const TMatrixD& m) -> std::vector<Double_t> {
(gdb) where
#0 QwCorrelator::CalcCorrelations (this=0x55555b7957e0) at /home/mrc/Github/PR191/Parity/src/QwCorrelator.cc:242
#1 0x00007ffff7cdff84 in QwDataHandlerArray::FinishDataHandler (this=this@entry=0x7fffffff7b00) at /home/mrc/Github/PR191/Parity/src/QwDataHandlerArray.cc:734
#2 0x0000555555565e68 in main (argc=<optimized out>, argv=<optimized out>) at /home/mrc/Github/PR191/Parity/main/QwParity.cc:560
|
@Mrc1104 Let me look at this again. Good points raised. |
Enhance QwRootNTuple with accessors for the RNTuple model and writer, and improve NTuple creation logging. Implement RNTuple support in QwCorrelator for field construction and filling, ensuring compatibility with existing functionality when RNTuple support is disabled.