Skip to content

Move to updated PID model, add track dt/dt fit info - #386

Merged
oksuzian merged 10 commits into
Mu2e:mainfrom
michaelmackenzie:TrkPID
Aug 7, 2026
Merged

Move to updated PID model, add track dt/dt fit info#386
oksuzian merged 10 commits into
Mu2e:mainfrom
michaelmackenzie:TrkPID

Conversation

@michaelmackenzie

Copy link
Copy Markdown
Contributor

This follows PRs Mu2e/MLTrain#9 and Mu2e/ArtAnalysis#8 to update the input PID model and to also add an optional branch with the track dt/dt fit results. I added rooutil information as well and a test script plotting the results.

@AndrewEdmonds11 AndrewEdmonds11 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great! Thanks, Michael. Could you also add the new branch to the ntuplehelper and recreate branches.md? Thanks

@michaelmackenzie

Copy link
Copy Markdown
Contributor Author

Looks great! Thanks, Michael. Could you also add the new branch to the ntuplehelper and recreate branches.md? Thanks

I just added this, thanks for reminding me!

@oksuzian

oksuzian commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Issues in PR #386:

  1. Branch misalignment risk (src/EventNtupleMaker_module.cc) — when the DtDt handle is invalid or shorter than the KalSeedPtr collection, nothing is pushed, so trkdtdt ends up shorter/misaligned with trk. RooUtil assumes 1:1. Fix: default-fill a TrkDtDtInfo for missing entries.

  2. handle->at(i_kseedptr) can throw — if the DtDt collection is valid but smaller than the seed collection.

  3. branches.md regression — regenerated from an older base; drops previously-merged Calorimeter/CRV/Trigger docs (field tables, calohitsmc alignment warning, trigger array explanation).

  4. Empty CRV explanation columnscrvpulses, crvdigis, crvpulsesmc rows now have blank explanations in branches.md.

  5. Wrong leaf linkcrvpulsesmc now points to CrvPulseInfoReco.hh (reco) instead of an MC struct.

  6. Missing trkDtDtTag on De fit config (fcl/prolog.fcl) — with fillTrkDtDt set globally true, fit branches lacking a trkDtDtTag get an empty tag → invalid handle → misaligned/empty branch.

@michaelmackenzie

Copy link
Copy Markdown
Contributor Author

Responding to the AI comments:

  1. I fixed this
  2. I now actively throw on this
  3. I merged in main and regenerated the documentation
  4. I don't know why this is missing
    @AndrewEdmonds11 I used ntuplehelper --list-all-branches --export-to-md >| doc/branches.md but it doesn't list these descriptions? Looking at these files it seems like they're missing this comment line, maybe it was done by-hand before?
  5. I added the missing tag

I think this should be merge ready, where another PR should fix the CRV object files, but I can also do this if preferred

@oksuzian

oksuzian commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

I'll review PR #386 to find issues. Let me gather the relevant data.Here's my review of PR #386 – Move to updated PID model, add track dt/dt fit info.

Summary

This PR updates the PID model and adds a new optional trkdtdt branch storing the track dt_hit/dt_trk fit results (slope, offset, uncertainty, chi², dof). It's an approved, low-risk PR (12 files, +225/−104) that follows upstream PRs in MLTrain and ArtAnalysis.

Reviews: 1 approval from @AndrewEdmonds11 with a request: "add the new branch to ntuplehelper and recreate branches.md." Both appear addressed in the current diff (helper/ntuplehelper.py and doc/branches.md are updated).

Issue found: silently mismatched dt/dt info when the collection is missing

This is the one substantive bug worth flagging before merge. In EventNtupleMaker_module.cc:

if(_conf.trk().fillTrkDtDt()) {
  auto handle = _allKSDtDtHs.at(i_trk_fit_branch);
  if(handle.isValid()) {
    if(i_kseedptr >= handle->size()) throw cet::exception("EventNtuple")
                                       << "Index " << i_kseedptr
                                       << " out of range of DtDt list size = "
                                       << handle->size();
    const auto& dtdt = handle->at(i_kseedptr);
    _infoStructHelper.fillTrkDtDtInfo(dtdt, _allTDtDtIs.at(i_trk_fit_branch));
  } else {
    const KalSeedDtDt dtdt; // empty dt/dt object
    _infoStructHelper.fillTrkDtDtInfo(dtdt, _allTDtDtIs.at(i_trk_fit_branch));
  }
}

The concern is the handle.isValid() == false branch. When fillTrkDtDt is true but the configured trkDtDtTag collection is absent (or mis-tagged), the code silently fills a default-constructed KalSeedDtDt (all zeros) for every track, rather than throwing. Contrast this with the valid-handle path, which does throw on any inconsistency (i_kseedptr >= handle->size()).

The result: a misconfigured tag produces an ntuple full of zero-valued trkdtdt entries that look like legitimate fit results (slope=0, offset=0, chisq=0, dof=0) with no indication anything went wrong. Since the user explicitly opted in via fillTrkDtDt: true, a missing collection is more likely a configuration error than an intended empty state.

Suggestions (pick one):

  • Throw (like the out-of-range case) when the handle is invalid but fillTrkDtDt is true, so misconfiguration fails loudly; or
  • Keep the graceful fallback but make the "no data" state distinguishable — e.g. initialize dof = -1 in TrkDtDtInfo so consumers can tell an unfilled entry from a real fit with dof=0. The rooutil example only null-checks track.trkdtdt (the pointer), not the fit validity, so a zeroed struct would be plotted as a real slope=0 entry.

Other minor observations

  • branches.md collateral edits: This PR reverts several previously-detailed sections (Calorimeter, CRV, Trigger docs) back to terser versions — e.g. crvpulses/crvpulsesmc descriptions become blank, and the detailed calohitsmc alignment warning is removed. If those richer docs landed in a separate recent PR, this may be an unintended merge/rebase regression rather than an intended doc change. Worth double-checking the base is current.
  • Whitespace noise: rooutil/inc/Track.hh is mostly trailing-whitespace cleanup (17/32 lines), which is fine but inflates the diff.

@oksuzian
oksuzian merged commit 84e7404 into Mu2e:main Aug 7, 2026
@michaelmackenzie
michaelmackenzie deleted the TrkPID branch August 7, 2026 02:37
@oksuzian oksuzian mentioned this pull request Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants