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.

[AndrewEdmonds11]

Response to AI review:

🔴 [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.

As the review says, this is covered by Mu2e/EventNtuple#381

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

Added. Good catch

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

Fixed. Good catch

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

Unclear what to do here. Do we have a cmake/spack build of ArtAnalysis?

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

Fixed. Good catch

🟡 [S2] The booster is never freed.

Fixed

⚪ [S3] Batch, none gating

Did all these clean ups

[oksuzian]

PR Review Summary — re-review of "Add TrkQual BDT to art module" (#7)

Reviewed at head c0dee801 (2026-08-07). Second pass. Prior review: 🔴 at a1920768 (review 4879365364, 7 findings). The delta is 10 commits, a1920768..c0dee801, touching two paths: TrkDiag/src/TrackQuality_module.cc (+33/−16) and a rename of TrkDiag/data/TrkQual_ANN1_v2.onnx → TrkQual_ANN1_v2.0.onnx. Every prior finding is accounted for below and verified in the code at this head, not taken from the reply comment. The PR is now mergeable: clean against main.

Decision

• 🔴 Request changes — narrowly, and not for anything carried over. Six of the seven findings are properly fixed and two of them go further than what I asked for. The blocker is new and comes from the fix to my own ⚪ nit about version naming: renaming the deployed ANN model file breaks every consumer of it, including the companion PR that is supposed to absorb this change. It is a one-line fix in either direction; I would flip to approve on it plus a plan for the merge ordering.

Scope understood (unchanged, plus the rename)

• TrackQuality runs the ONNX ANN and an XGBoost BDT off one feature vector, publishing TrkQual:ANN and TrkQual:BDT.
• New this pass: feature-count guards for both models, bdt_score zeroing on tracks without a tracker-entrance intersection, cet::exception throughout, a destructor freeing the booster, dead TMVA/SOFIE code removed, and the ANN model file renamed to the dotted form.

---

Findings

1. 🔴 [S0] The ANN model file was renamed, and nothing that reads it was updated — including the companion PR.

   • Evidence: TrkDiag/data/ at c0dee801 contains TrkQual_ANN1_v2.0.onnx; TrkQual_ANN1_v2.onnx no longer exists (commit 399db041, "Consistent version numbering (AI suggestion)"). Every consumer still names the old file:
     • Mu2e/EventNtuple fcl/prolog.fcl:17 (main) — onnxFilename : "ArtAnalysis/TrkDiag/data/TrkQual_ANN1_v2.onnx"
     • Mu2e/EventNtuple fcl/prolog.fcl:12 on the #381 branch (151e5beb) — same string; #381 added xgbFilename for the BDT but left the ANN path untouched
     • Mu2e/EventNtuple fcl/from_mcs-mixed_trkQualCompare.fcl
   • Impact: ConfigFileLookupPolicy cannot resolve the path, so the Ort::Session member initialiser throws at module construction and every job configuring TrkQual dies before the first event. Critically, this is not covered by "the companion handles it" — #381 as it stands today would still be broken, so the two PRs are no longer sufficient even when merged together.
   • Suggested fix, and my apologies for pointing at the riskier of the two directions: the naming inconsistency I flagged is better resolved at the source, by setting training_version = "2" in MLTrain's notebook so future exports match the already-deployed TrkQual_ANN1_v2.onnx, leaving the committed artefact untouched. That also fixes the MLTrain#8 README, whose cp command names v2. If you prefer to keep the rename, then all three fcl references above must change in the same lockstep — and #381 needs a commit for it.
   • Note the BDT side is already right: TrkQual_BDT1_v2.0.ubj matches what #381 configures.

2. 🟡 [S2] xgboost is still in the scons build only — carried over, with an answer to your question.

   • You asked: "Unclear what to do here. Do we have a cmake/spack build of ArtAnalysis?" The evidence says the CMake build exists but is not exercised:
     • It is defined: top-level CMakeLists.txt has find_package(cetmodules), project(ArtAnalysis), find_package(Offline REQUIRED EXPORT), and TrkDiag/CMakeLists.txt carries an explicit cet_build_plugin entry for every module including TrackQuality (:141-151).
     • It is already broken for this plugin: TrkDiag/CMakeLists.txt contains no onnxruntime, no xgboost, no openblas and no ROOTTMVASofie anywhere — while TrkDiag/src/SConscript lists all four (:53, :106-107, mainlib :45-54). So the ONNX path has been unlinkable in CMake since #4 merged on 2026-06-18 and nobody noticed, which is fairly direct evidence that nothing builds ArtAnalysis this way today.
     • The repo is a muse musing — an (empty) .muse marker at the top level — so muse build drives scons, which is the path your change correctly updates. ArtAnalysis also has no CI on either build system.
   • Impact: none today, on this evidence. Raising it only to close the question, not to gate.
   • Suggested fix: either add the one line to TrkDiag/CMakeLists.txt alongside the missing onnxruntime/openblas/ROOTTMVASofie so the two files stop diverging, or — if the CMake build is genuinely dead for this repo — say so in the README and stop maintaining cet_build_plugin entries. The half-maintained state is what turns a missing dependency into a silent trap.

3. ⚪ [S3] Small residuals in the new code.

   • _configFileLookup(conf().xgbFilename().c_str()) — the .c_str() is redundant; ConfigFileLookupPolicy::operator() takes a std::string, and the ONNX call three lines up passes one directly.
   • Now that initializeMVA and _printMVA are gone, #include "Offline/Mu2eUtilities/inc/MVATools.hh" looks unused; art_root_io/TFileService.h and art/Utilities/make_tool.h likewise have no remaining user in this file.
   • The PrintMVA fhicl atom survives in Config — check whether anything still reads it after the TMVA cleanup.

---

Carry-forward accounting (vs review 4879365364 at a1920768)

1. 🟠 [was S0] Cross-repo break — PARTIAL, and now larger. You are right that Mu2e/EventNtuple#381 is the companion and that my review said so; the finding was never that no companion existed, but that it is not merge-ready and that the dependency is mutual. That still holds: #381 has its own open blocker from my review of it (the trkqual_metadata histogram gets unlabelled bins when trk.fill : false, which makes RooUtil throw on those files), and merging either PR alone still aborts jobs — this one makes xgbFilename required where EventNtuple main does not set it, #381 sets a key this repo's main does not recognise. Finding 1 above now adds a third item to the same lockstep. Downgrading S0→S1 only because the coordination is understood and tracked; it is not resolved.
2. 🟢 [was S1] Feature-count verification — FIXED in 06a811a9, verified, and extended. :135 now compares nFeaturesModel != _nFeatures and throws with both counts. You also added the equivalent guard for the ANN at :114 (_total_size != _nFeatures), which I had suggested only in passing — that closes the second independent statement of "7" I flagged in the same finding.
3. 🟢 [was S1] BDT score on tracks with no tracker-entrance intersection — FIXED in 920c14f9, verified. :268-269 sets bdt_score = 0 under the same if (!entrance_found) that zeroes the ANN at :240, so the two products now agree on those tracks.
4. 🟢 [was S2] std::runtime_error → cet::exception — FIXED in 53b9e64a, verified. All seven throw sites now use cet::exception("TrackQuality"); no std::runtime_error remains in the file.
5. 🟢 [was S2] Booster leak — FIXED in ea71e03a, verified. ~TrackQuality() declared at :58 and defined at :142 as { if (_booster) XGBoosterFree(_booster); }.
6. 🟢 [was S3] Batch — DONE. _configFileLookup is reused instead of a throwaway policy (1ab1271e); the member and fhicl key are both xgbFilename (483421bd); the debug printf's stray m suffixes are gone and the two outputs are comma-separated (c88dd784); the size-consistency check now covers bdtcol as well as anncol (8ed7933f, :285 and :288); and c0dee801 removed the dead initializeMVA declaration, the write-only printMVA_, and the stale "using TMVA::SOFIE" header comment. Only the items in finding 3 above remain.
7. 🟡 [was S2] Build-system asymmetry — UNADDRESSED, now finding 2, with the answer to your question.

---

Verified 🟢 — no action needed

• 🟢 The per-track XGBoost resource handling remains correct: XGDMatrixFree(dmat) is still called on the success path and both error paths before throwing.
• 🟢 The prediction output is still defended (out_len < 1 || out_result == nullptr before reading out_result[0]).
• 🟢 The design claim still holds — one features vector feeds both Ort::Value::CreateTensor and XGDMatrixCreateFromMat, so the two models see identical inputs.
• 🟢 The new .ubj needs no build-file change: TrkDiag/CMakeLists.txt:172 installs the data directory wholesale, and scons resolves it from the source tree.
• 🟢 Threading unchanged: art::EDProducer (legacy), so produce is serialised and the shared BoosterHandle is never entered concurrently.
• 🟢 The PR is now mergeable: clean against main — the ArtAnalysis#8 merge and the CaloGeomUtil follow-ups did not collide with this branch.

Validation check

• Build/tests run: none — ArtAnalysis has no CI on either build system, the PR body still carries no build or run evidence, and this review did not compile the code. All findings are static, against this head and against EventNtuple main / the #381 branch via the GitHub API.
• Config contract check: fail — finding 1 (renamed model file with no consumer updated). The fhicl schema itself is coherent, and xgbFilename is correctly required.
• Cross-repo consistency: fail pending the lockstep — #381 must gain the ANN filename update, resolve its own blocker, and land in the same musing build as this PR.

Residual risk

• The BDT's physics is still unvalidated in this PR — no ROC, no ANN-vs-BDT comparison, and no guidance on which score analyses should use, while both ship with equal standing. A single overlay in the PR body would close it.
• With the feature-count guards in place, a retrained model with a different feature set now fails loudly at construction instead of silently scoring garbage. That was the main silent-wrong-answer path and it is gone.

Author follow-ups

1. Resolve the model-filename split (finding 1, blocking): preferably revert the artefact rename and fix MLTrain's training_version to "2" instead; otherwise update fcl/prolog.fcl and from_mcs-mixed_trkQualCompare.fcl in EventNtuple#381 in the same pass.
2. Decide what to do about TrkDiag/CMakeLists.txt — add the four missing external libraries, or declare the CMake build unsupported for this repo (finding 2, non-gating; my read of the evidence is in the finding).
3. State the intended merge order with EventNtuple#381 in both PR bodies, given the dependency is mutual (carry-forward 1).
4. Optional: the three residuals in finding 3.
5. Still worth having: a ROC / performance comparison for the BDT in the PR body.