Add TrkQual BDT to art module

[AndrewEdmonds11]

Following Sam's CrvInference module, I have added the TrkQual BDT algorithm to the TrackQuality module. The TrackQuality module now runs both algorithms and produces two output data products:

• TrkQual:ANN
• TrkQual:BDT

I decided to run both algorithms in the same module to ensure that they both use the exact same features when evaluating.

[oksuzian]

PR Review Summary — "Add TrkQual BDT to art module" (#7)

Reviewed at head a1920768 (opened 2026-06-30, single commit). First review of this PR; no prior reviews or comments. Scope: run an XGBoost BDT alongside the existing ONNX Runtime ANN inside TrackQuality, and emit two MVAResultCollection products instead of one. 3 files, +68/−12: TrkDiag/src/TrackQuality_module.cc, TrkDiag/src/SConscript, and a new TrkDiag/data/TrkQual_BDT1_v2.0.ubj.

Decision

• 🔴 Request changes. The design intent is right — evaluating both models from one feature vector in one module is exactly how you guarantee they see identical inputs, and the code achieves that. But merging this as it stands breaks the standard EventNtuple ntupling path in two ways with no companion PR (S0), the model/feature-count consistency check is written but never actually performed (S1), and the two products disagree on tracks with no tracker-entrance intersection because the "not a good track" override is applied to the ANN only (S1).

Scope understood

• TrackQuality_module.cc: adds #include <xgboost/c_api.h>, a BoosterHandle member loaded in the constructor from a new required xgbFilename parameter, per-track XGDMatrixCreateFromMat → XGBoosterPredict → XGDMatrixFree on the same 7-element features vector the ANN uses, and splits the single unnamed output product into "ANN" and "BDT" instances.
• TrkDiag/src/SConscript: 'xgboost' appended to the helper.make_plugins([...]) link list.
• TrkDiag/data/TrkQual_BDT1_v2.0.ubj: the trained booster, in XGBoost's UBJSON format.
• Not touched: TrkDiag/CMakeLists.txt, and anything in Mu2e/EventNtuple — which is where every consumer of this module lives.

---

Findings

1. 🔴 [S0] Merging this alone breaks the standard EventNtuple path twice over; the companion PR exists but is not merge-ready, and the two are mutually blocking.

   • Evidence, break (a) — loud: fhicl::Atom<std::string> xgbFileName{Name("xgbFilename"), Comment("Path to XGBoost .ubj model file")} has no default, so it is mandatory. EventNtuple/fcl/prolog.fcl:10-15 configures the shared TrkQual table with exactly module_type, onnxFilename and debugLevel. Every one of the ten producers derived from it (TrkQualDeM … TrkQualDe, TrkQualProducers) therefore fails fhicl validation at module construction as soon as the next Analysis musing picks this up.
   • Evidence, break (b) — silent, and worse: the module now publishes only label:ANN and label:BDT; nothing is published under the empty instance name any more. EventNtuple asks for bare labels — trkQualTags : ["TrkQualDeM"] (fcl/prolog.fcl), ["TrkQualReflecte"] (fcl/from_mcs-reflection.fcl), ["TrkQualAllV10", …] (fcl/from_mcs-mixed_trkQualCompare.fcl) — which EventNtupleMaker_module.cc:929-933 converts to an art::InputTag with an empty instance and fetches with event.getByLabel(...), no validity check at fetch time. The fill site at :1366-1372 is guarded by if(trkQualHandle.isValid()), so a missing product does not throw: the trk.qual branch is simply written with default-constructed values, for every track, in every event. An analyst gets an ntuple that looks complete and has no TrkQual in it.
   • Impact: (a) aborts jobs; (b) corrupts a headline analysis branch without any diagnostic. Same class of issue as ArtAnalysis#8's TrackPID instance-name change, and worth solving the same way in both.
   • The companion, for the record: Mu2e/EventNtuple#381 ("TrkQual BDT") does the other half — it replaces trkQualTags with a trkQualLeaves table carrying TrkQualDeM:ANN-style tags, and adds xgbFilename to the TrkQual prolog. This PR's body does not mention it (#381's body mentions this one). Two problems remain even so: #381 reports mergeable: false against main and has its own blocker, so it cannot land today; and the dependency is mutual — merging #381 first is equally fatal, because xgbFilename is not a key that this repo's main recognises, so fhicl validation rejects the prolog. There is no merge order that does not break jobs in the window between the two.
   • Suggested fix: reference #381 in this PR's body and state that both must land inside the same Analysis musing build. Better, remove the window entirely: keep the ANN on the unnamed instance (produces<MVAResultCollection>() unchanged), add only "BDT" as a named instance, and give xgbFilename a default. Then this PR merges harmlessly on its own, #381 follows at leisure, and no config is ever briefly invalid. Worth considering given how many fcl files reference these labels.

2. 🟠 [S1] The feature-count verification is written but never performed — the one guard against a model/code mismatch does nothing.

   • Evidence, constructor:

     // verify the loaded model matches the expected feature count
     bst_ulong nFeaturesModel = 0;
     if (XGBoosterGetNumFeature(_booster, &nFeaturesModel) != 0) {
       throw std::runtime_error(std::string("XGBoosterGetNumFeature failed: ") + XGBGetLastError());
     }

     nFeaturesModel is never compared to anything, and never read again. Only the call's return code is checked. Because it is passed by address the compiler sees it as used, so no -Wunused warning fires.
   • Impact: _nFeatures is a hardcoded static constexpr size_t _nFeatures = 7 carrying the comment "Number of features is fixed, must match training!", and it is passed straight to XGDMatrixCreateFromMat(features.data(), 1, _nFeatures, NAN, &dmat). Point xgbFilename at a booster trained on a different feature set — which is exactly what happens on the next retraining, since the file is a configurable path — and XGBoost is handed a 7-wide row for an n-wide model. Depending on n that is a silently wrong score or an out-of-bounds read of the features buffer. The check that would have caught it is right there, one if short of working.
   • Suggested fix:

     if (nFeaturesModel != _nFeatures) {
       throw cet::exception("TrackQuality") << "XGBoost model expects " << nFeaturesModel
         << " features but the module supplies " << _nFeatures;
     }

     Consider doing the same for the ANN: _total_size is derived from the ONNX input shape and is the second independent statement of "7" in this file, never cross-checked against _nFeatures.

3. 🟠 [S1] Tracks with no tracker-entrance intersection get an overridden ANN score but a raw BDT score.

   • Evidence: when the TT_Front intersection is not found, the module sets features[2] = -9999 and features[5] = -9999 (t0 error and momentum error sentinels), runs both models on that vector, then applies

     if (!entrance_found) {
       annout[0] = 0; // this is not a good track
     }

     There is no equivalent for bdt_score, which is stored as whatever the booster returns for a row containing two −9999 sentinels.
   • Impact: for the same track, TrkQual:ANN says 0 ("not a good track") while TrkQual:BDT reports a score the model never saw a training analogue for — the two products are inconsistent by construction, precisely on the pathological tracks a quality variable exists to catch. Sentinel values that far outside the training range are also the classic way to get a confidently high BDT score from an untrained corner of feature space. The PR body's rationale ("to ensure that they both use the exact same features") makes the asymmetry harder to notice, not easier: the features are identical, the post-processing is not.
   • Suggested fix: hoist the guard — if !entrance_found, set both scores to 0 and skip both inferences, e.g.

     if (!entrance_found) { anncol->push_back(MVAResult(0)); bdtcol->push_back(MVAResult(0)); continue; }

     If instead the BDT is meant to handle the sentinels itself, say so in a comment and drop the ANN override for symmetry — but then the sentinel values need to have been in the training set.

4. 🟡 [S2] xgboost is added to the scons build only, and I could not confirm either external is available to the build at all.

   • Evidence: TrkDiag/src/SConscript gains 'xgboost' in helper.make_plugins([...]). TrkDiag/CMakeLists.txt:141-151, cet_build_plugin(TrackQuality art::module ...), lists only ArtAnalysis::TrkDiag and five Offline::* targets — no xgboost, and (pre-existing, from #4) no onnxruntime either, while cet_make_library at the top of the same file lists neither. The repo's top-level CMakeLists.txt has no find_package for either.
   • Separately, and stated as a limitation rather than a finding: I could not locate libxgboost or libonnxruntime anywhere under /cvmfs/mu2e.opensciencegrid.org/{packages,artexternals,spackages} on this node. Since onnxruntime demonstrably works (ArtAnalysis#4 is merged), my probe is inconclusive rather than evidence of absence — but it does mean I cannot verify that xgboost is provided by the current stack, and neither can CI, because ArtAnalysis has none.
   • Impact: if CMake/spack is a supported build for ArtAnalysis, this plugin does not link there; if it is not supported, the two build files have been drifting since #4 and this PR widens the gap. Either way the dependency situation is not stated anywhere.
   • Suggested fix: mirror the dependency in cet_build_plugin(TrackQuality ...) (and add onnxruntime while you are there), and put the build evidence in the PR body — which musing/release provides xgboost, and the muse build output for this branch. If xgboost is newly needed in the stack, that is a spack/musing request that must land first.

5. 🟡 [S2] The new error paths use std::runtime_error instead of cet::exception.

   • Evidence: six new throw sites (XGBoosterCreate, XGBoosterLoadModel, XGBoosterGetNumFeature, XGDMatrixCreateFromMat, XGBoosterPredict, XGDMatrixFree, plus "returned no result") all throw std::runtime_error. The pre-existing code in the same file uses throw cet::exception("TrackQuality") << ....
   • Impact: the Mu2e coding standard asks for cet::exception with a meaningful category, and art's error handling formats and categorises those; a bare std::runtime_error from module construction surfaces without the module context that makes a production failure diagnosable. Mixing both idioms in one file also invites the next contributor to pick either.
   • Suggested fix: convert all seven to cet::exception("TrackQuality") << "XGBoosterLoadModel failed: " << XGBGetLastError();.

6. 🟡 [S2] The booster is never freed.

   • Evidence: XGBoosterCreate(nullptr, 0, &_booster) in the constructor, no destructor and no XGBoosterFree(_booster) anywhere in the file. (The DMatrixHandle per track is correctly freed on all three paths — that part is right.)
   • Impact: bounded, not a per-event leak — one booster per module instance — but the standard EventNtuple path constructs ten TrkQual producers, each holding an XGBoost model for the life of the job. It is also the kind of omission that a ~TrackQuality would have made obviously correct.
   • Suggested fix: add ~TrackQuality() { if (_booster) XGBoosterFree(_booster); }, or wrap the handle in a std::unique_ptr with a custom deleter.

7. ⚪ [S3] Batch, none gating:

   • std::string modelPath = ConfigFileLookupPolicy()(conf().xgbFileName()); constructs a throwaway policy while the class already holds _configFileLookup, which is what the ONNX path uses eight lines earlier.
   • The config member is xgbFileName but the fhicl key is "xgbFilename"; the ONNX pair is onnxFilename/"onnxFilename". Match the capitalisation so grep finds both.
   • The debug printf gained a stray unit: "--> ANN output = %.4fm BDT output = %.4fm\n" — two spurious ms, and no separator before "BDT".
   • The size-consistency check still tests anncol only; bdtcol is filled in the same loop so it cannot differ, which is an argument for checking neither rather than one.
   • TrkQual_BDT1_v2.0.ubj vs the existing TrkQual_ANN1_v2.onnx — v2.0 against v2 for what the PR body describes as the same training round. Worth settling in MLTrain (the notebook's training_version = "2.0" is what produces the dotted form) so the two artefacts agree.
   • Pre-existing in this file, now more visible: the header comment still says "using TMVA::SOFIE"; initializeMVA(std::string) is declared and never defined or called; _printMVA is assigned in the constructor and never read.

Verified 🟢 (checked, no action needed)

• 🟢 The core design claim holds. A single std::vector<float> features is filled once per track and handed to both Ort::Value::CreateTensor and XGDMatrixCreateFromMat, so the two models genuinely see identical inputs — the stated reason for one module rather than two, delivered.
• 🟢 The per-track XGBoost resource handling is correct: XGDMatrixFree(dmat) is called on the success path and on both error paths before throwing, so the matrix does not leak even when prediction fails.
• 🟢 The prediction output is defended: out_len < 1 || out_result == nullptr is checked before out_result[0] is read.
• 🟢 The new data file needs no build-file change. TrkDiag/CMakeLists.txt:172 is install(DIRECTORY data DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/ArtAnalysis/TrkDiag) — directory-based, so TrkQual_BDT1_v2.0.ubj is installed automatically, and ConfigFileLookupPolicy resolves it the same way as the .onnx.
• 🟢 Threading is not a concern here: TrackQuality derives from art::EDProducer (legacy), so art serialises produce calls and the shared BoosterHandle is never entered concurrently.
• 🟢 Nothing in Production or mu2e-trig-config configures TrackQuality; ArtAnalysis itself ships no fcl for it either (the only in-repo mentions are the module source and CMakeLists.txt). EventNtuple is the sole consumer — which is what makes finding 1 the whole cross-repo story.
• 🟢 PR hygiene: single topic, and the body states the intent and the design rationale.

Validation check

• Build/tests run: none — ArtAnalysis has no CI on either build system, the PR body carries no build or run evidence, and this review did not compile the code. Static verification against ArtAnalysis/EventNtuple/Offline at their current heads via the GitHub API, plus a survey of the cvmfs package trees for the two externals.
• Config contract check: fail — a new required fhicl key with no consumer updated (finding 1a), and a product-identity change that no consumer's config reflects (finding 1b).
• Cross-repo consistency: fail — the companion (EventNtuple#381) exists but is not merge-ready, and the dependency is mutual in both directions (finding 1). Note also that EventNtuple/fcl/from_mcs-mixed_trkQualCompare.fcl still sets datFilename and points at Offline/TrkDiag/data/*.dat — already stale on main since ArtAnalysis#4 replaced that key with onnxFilename; #381 fixes the key but points the result at v1/v1.1 .onnx files that do not exist in this repo, so that example still will not run.

Residual risk

• Finding 1b is the one to worry about: it produces a plausible-looking ntuple with an empty quality branch, and nothing in the job says so. If the companion PR lands late, that window is silent.
• The BDT's score is unvalidated in this PR — no ROC, no ANN-vs-BDT comparison, no statement of which one analyses should use, and both are written to the event with equal standing. A plot in the PR body would settle it.
• _nFeatures, _total_size and the trained model each independently assert "7 features". Findings 2 and 7 reduce that to one source of truth; until then a retraining that adds a feature is a silent-wrong-answer scenario rather than an error.

Author follow-ups

1. Reference the companion Mu2e/EventNtuple#381 in this PR's body and state that the two must land in the same musing build — or, better, keep the ANN on the unnamed instance and default xgbFilename, which removes the mutually-breaking window entirely (finding 1).
2. Actually compare nFeaturesModel against _nFeatures and throw on mismatch (finding 2).
3. Decide and implement what the BDT should report when the tracker-entrance intersection is missing — currently the ANN is zeroed and the BDT is not (finding 3).
4. Mirror the xgboost dependency in TrkDiag/CMakeLists.txt, and state in the PR body which release provides xgboost plus the muse build output for this branch (finding 4).
5. Convert the new std::runtime_error throws to cet::exception("TrackQuality") (finding 5), and free the booster in a destructor (finding 6).
6. Optional: the S3 batch in finding 7 — especially the v2.0/v2 artefact naming, which is worth agreeing with MLTrain before more models land.