[dylondef]

Describe your changes

Adds the production cMrdMlReco UserTool for cMRD machine-learning reconstruction and integrates its output into PhaseIITreeMaker.

cMrdMlReco runs the frozen cMRD v5 gate and reconstruction SavedModels through a persistent Python inference daemon and publishes the reconstruction through Stores["MrdMLReco"].

The deployed SavedModels and geometry remain on PNFS. The ToolAnalysis adapter, frozen runtime source, example configuration, README, and parity checker are versioned with the UserTool so the reconstruction interface and inference code are reviewable.

PhaseIITreeMaker now supports:

• MRDRecoSource Legacy — existing legacy MRD reconstruction
• MRDRecoSource MrdMLReco — native mrd_ml_* output only
• MRDRecoSource Both — legacy and ML reconstruction side by side

The default remains Legacy.

A separate commit makes several event-level inputs optional for lean ToolChains and prevents triggers without MrdTimeClusters from being dropped when MRDRecoSource=Both. This commit is intentionally separated because it changes existing PhaseIITreeMaker behavior for triggers without MRD clusters.

Validation

Data validation was performed on ANNIE Run 5898:

• 213,669 phaseIITriggerTree entries were preserved.
• The production output contained the expected reduced mrd_ml_* branch set and exactly one mrd_ml_metadata entry.
• All 46 shared physics branches matched the commissioned B2 reference exactly, event-by-event, across all 213,669 events.

A WCSim production smoke test was also performed:

• 729 events written successfully.
• 62 events passed the cMRD gate.
• 46 events had reconstruction available.
• 0 events had a nonzero mrd_ml_failure_code.
• The output ROOT file contained the expected production branches and one mrd_ml_metadata entry.

A GENIE teardown segfault occurred after ToolAnalysis had already finalized during the WCSim test; the output ROOT file was healthy and the crash stack was in GENIE process-exit cleanup rather than cMrdMlReco.

Checklist before submitting your PR

• This PR implements a single change (one new/modified Tool, or a set of changes to implement one new/modified feature)
• This PR alters the minimum number of files to affect this change
• If this PR includes a new Tool, a README and minimal demonstration ToolChain is provided
• If a new Tool/ToolChain requires model or configuration files, their paths are configuration-driven rather than hard-coded in the C++ source, and the README describes the required PNFS release artifacts
• For every new usage, there is a reason the data must be on the heap
• For every new there is a delete, unless I explicitly know why ownership is transferred

Additional Material

The cMRD release identity, model hashes, geometry hash, gate thresholds, runtime versions, and output mode are written to a one-entry mrd_ml_metadata tree for provenance.

[marc1uk]

is that not something which can be switched automatically?
if(MRDRecoSource!=Legacy) TriggerProcessing=1; ?

[dylondef]

yeah, I think that makes sense. It is a little redundant to have the user remember to switch that separately, so I can set TriggerProcessing = 1 automatically whenever MRDRecoSource is not Legacy.

[marc1uk]

Adding 200 new standalone variables is ... just really, really not good.
Please put these into a ✨ class ✨

[dylondef]

yeah, my bad. I grouped the loading,reset,branch logic but left the actual branch backing variables flattene. I can move those into a single MrdMLBranchesclass and keep the existing flatmrd_ml_*` ROOT branch names unchanged.

[marc1uk]

Thanks. Nothing personal, it's following the same standard that everyone else has done in the Tools, it's just a bad practice that is cumulatively getting worse with each new addition.

[marc1uk]

There is no separate 'PhaseIITreeMaker patch 0003'.

[dylondef]

yeah, that is just a leftover reference to the numbered patch files I was using internally while debugging the changes. I’ll remove the patch reference and just document the lean chain requirement directly.

[dylondef]

Theres going to be a bit of that through out here ill need to make a version of comments just for release then

[marc1uk]

why is this a configuration option? Surely it should be defined by the system?

[dylondef]

I exposed it originally so the daemon could be pointed at a specific TensorFlow environment, but I agree that normally this should come from the system. I'll remove it from the config.

[marc1uk]

Thanks, I have personally not had good experiences with running parallel python environments (admittedly I'm not a python developer), so I would really prefer to just keep things simple with one standard environment within the ToolAnalysis container.

[marc1uk]

Move this to a method associated to the class encapsulating these 200 variables. It takes a tree, makes the branches, pointed at its members.

[marc1uk]

why not just set TriggerProcessing=1?

[dylondef]

yeah i was being to strict with it for no reason, ill swap it to the set

[marc1uk]

Factor these into a function.

[dylondef]

okay. This should fall out naturally from the cMRD branch class refactor, I’ll move the specific setup/validation into a helper rather than keeping the whole block with Initialise().

[marc1uk]

you control the source code for the publishment. Just use the right case there.

[dylondef]

yeah thats leftover when i was using debug/smoketest calling it different things and testing ill remove it

[marc1uk]

thanks. Again, just trying to streamline the code, it's already excessively long (not, by large part, due to your additions).

[marc1uk]

if got_data_streams is false, this block never runs, and fHasTank and fHasMRD never get set, in contrast with previous behaviour. Is that intended?

[dylondef]

good catch, that change wasn’t on purpose. I was trying to make DataStreams optional for the leaner version, but that does change the fallback value of fHasTank/fHasMRD when it is missing. This doesn’t affect the cMRD reconstruction itself, just what PhaseIITreeMaker writes. Would you prefer that I keep it as 0, or leave an explicit sentinel when its unavailable?

[marc1uk]

I'm not really a good person to ask, being largely uninvolved in analysis and using the outputs. I would default to retain existing behaviour, but by all means pose the question at a software meeting and take account of actually relevant opinions there.

[marc1uk]

if the Get fails,... is it ok to carry over the existing fEventTimeMRD_Tree value?
admittedly this is the same as prior behaviour, just checking if that's appropriate - generally an if on a get should have an associated else.

[dylondef]

ResetVariables() should take care of that by resetting fEventTimeMRD_Tree to the sentinel before this block, but I can add the explicit else assignment here as a failsafe as well

[marc1uk]

Unless the source is doing m_data->CStore.Delete("MrdTimeClusters") before setting the new value, an event that has no time clusters for this event will presumably just return true, but MRDTimeClusters.empty() will be true - should this be
if(!get_clusters || MRDTimeClusters.empty())?

[dylondef]

yeah I think that change works, I originally left it separate as a check in case TimeClustering had been omitted, but theres the MRDClusterProcessing tag that already catches that earlier so combining the empty versus missing cases makes sense.

[marc1uk]

make 'Reset' member function of the class encapsulating these variables

[marc1uk]

nice

[marc1uk]

Move these into a 'Get' member function of the encapsulating class

[marc1uk]

👍

[marc1uk]

this could also be a member function of variable encapsulation class.
PhaseIITreeMaker is over 3,000 lines of code - we really need to not be adding more.

[dylondef]

Yeah, I'll work on encapsulating the cMRD variables and functions into something like MrdMLTreeData.h/.cpp. I was thinking of following the existing MRDTrackLib style class approach, keeping it as a helper called by PhaseIITreeMaker rather than introducing another ToolChain Tool. If youre okay with that or would you prefer something else?

[marc1uk]

That would be a good compromise I think, for the moment. To be honest the PhaseIITreeMaker being a single convergence point of numerous other Tools is fundamentally a ToolChain antipattern - it couples one Tool to numerous others, breaking modularity, meaning changes in source Tools need to be mirrored by changes in the PhaseIITreeMaker, and produces the observed huge, monolithic central point.
The 'correct' approach would be a TTree in the DataModel, onto which each Tool creates and fills its own branches. The 'TreeMaker' Tool would simply be responsible for making the file and tree in Initialise, calling 'TTree::Write' in Execute, and closing the file in Finalise. Alas, at this point implementing both strategies would probably just be even more fragmented and confusing. 😔

[marc1uk]

V6? B2? This the first PR of this Tool but documentation seems to be referencing a history we've never seen.

[dylondef]

yeah my bad, thats my own notes I should of been better when i was reading through my own comments and note taking, as you have see with the python tho it gets to be a bit much, ill fix this up tho

[marc1uk]

This is some seriously low-level stuff here. Is this really necessary??? This feels like it's been hugely over-engineered...

[dylondef]

Ill probably tackle the response to your bigger comment as I feel it ties in with that, but yeah, I think that's fair. A lot of the extra validation and low-level stuff came out of debugging the model, geometry, and C++/Python interface issues I ran into during development. I was hesitant to remove too much before the PR review since I've been the only one working on this, and I wanted another set of eyes on what was actually necessary versus what could be simplified.

[marc1uk]

Is there any documentation on performance comparison between the new and legacy reconstruction?
Was the documentation included mayhaps (heavily) AI-assisted in generation? It reads like word salad. 🥗

This commit is intentionally separated because it changes existing PhaseIITreeMaker behavior for triggers without MRD clusters.

I'm not sure exactly what you mean about separating some changes into a different commit. When merging PRs the set of commits are generally squashed into a single commit, so what changes the original branch commits made is a bit meaningless. Unless you're anticipating the changes in a particular commit needing a rollback, what changes were part of what commit is pretty moot.
Secondly it sounds like as long as the configuration variable MRDRecoSource=Legacy, the behaviour is unchanged? If so, there shouldn't be a need to rollback to reproduce the old behaviour, right?

A GENIE teardown segfault occurred after ToolAnalysis had already finalized during the WCSim test; the output ROOT file was healthy and the crash stack was in GENIE process-exit cleanup rather than cMrdMlReco.

Can you try running the same toolchain without the cMrdMlReco Tool (just comment it out in the ToolsConfig) and see if you get the same issue? If not it may be that the new Tool is introducing memory corruption, even if the segfault appears to point elsewhere.

Speaking of which, please provide a reference minimal ToolChain that runs the Tool to demonstrate required inputs and configuration files. In fact, you have provided a configuration file, but it's in the UserTool directory - please move that to the corresponding location in configfiles.

What exactly is the purpose of the hashes... I get the impression this is because a given inference model was trained on, and subsequently requires the runtime loading of, a particular geometry, gate model and reconstruction model tree. If these four elements are coupled, they should probably just all be put into a single directory in /pnfs/ and the directory to that path given as the sole configuration input. Specifying 5 different directories and 5 different hashes to ensure that the 5 different directories all match is just... overcomplicating things. Why not provide one root directory and use relative paths for the things inside it?

ok, moving onto the code:
The python code makes imports of

• future
• hashlib
• json
• math
• dataclasses
• pathlib
• typing
• numpy
• argparse
• traceback
• awkward
• uproot

Sure is python! Step 1: import the universe. 🙃
Does the ANNIE ToolAnalysis container have all these installed? If not (which i suspect) they will need to be added, with version numbers, to the GetToolDAQ.sh script and compatibility tested with the existing imports from other python Tools. That'll then need to be turned into an updated docker container.

OK, onto the actual crux of the matter. I'm afraid I struggled to read all 4,000 lines of python and 2,000 lines of c++ in your main tool. I went a bit cross-eyed. While well structured, there is extensive low-level functionality. It seems like you're parsing all the relevant data into JSON strings in order to pass them between c++ and python, with extensive validation. It's ... a lot.

ToolAnalysis is an application based on ToolChains. Each ToolChain contains a set of standard-format Tools, with an Initialise, Execute and Finalise method. Tools can be c++ or python (e.g. RingCounting, or DNNTrackLengthPredict.py )
Why does this require a hand-rolled forked python process and locale-conditioned JSON-based inter-process pipe for sending data as strings.....
This is going to be basically impenetrable for any other developers to maintain or debug if anything changes in the future.
While I don't like to reject what clearly looks like a lot of work, I'm equally reluctant to merge what is essentially a completely unrelated application and an embedded API to communicate with it.
What is the feasibility to refactor this into our standard python Tool format?

[dylondef]

Thanks for taking the time to review all of this, I agree the current implementation is more complicated than it could be for a long term ToolAnalysis Tool.

For some context, I've spent about nine months developing and integrating this reconstruction model mostly working on it myself. I ran into quite a few issues with geometry, timing, model inputs etc. The cpp/python interface grew out of debugging those problems. Having the json interface made it esier to isolate errors and verify what was actually being passed to the models. It worked well for development, but I agree it left behind a lot of extra code.

I also spent some time making sure the models were compatible with the packages already available in ToolAnalysis, I built a matching Python 3.8/tensorflow 2.10 environment on my training server rather than requiring a new environment for inference. I did just double check the ANNIE Apptainer contrainer again, and TesnorFlow 2.10, numpy and pyroot are already installed and import together successfully. Some of the other imports are standard but also are just used for the development like uproot and akward we can swap for just pyroot.

For the comparison with FindMrdTracks, I present the MC reconstruction and recovery results at the Fall 2026 Collaboration meeting the presentation is here https://annie-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=6895 , and i will be showing a data one here soon.

And yes, portions of the code review and documentation were AI-assisted. I went through the code myself it does take a few days to go through all of it so I understand the eye-cross, but also used the AI to help check and document it. I agree some of the comments became unnecessarily long, and I'll clean those up. I should of went back over it after I asked it to clean up my own language as sometimes I talk very direct and roughly.

On the commits, the separation was mainly for my own debugging and testing. You're right that it doesn't matter much once everything is merged. MRDRecoSource=Legacy is intended to preserve the original PhaseIITreeMaker behavior. I'll double-check that against the original output, especially for triggers without MRD clusters.

For the Genie segfault, the wcsim test completed ToolAnalysis finalization for ~2000 part files so far and wrote the expected Root output before the crash, I think its just my version of rewieghtgenie doesnt like to close itself that well, I have tested it just with the base tools like FindMrdTracks and simplereco and got the same closure error.

Yeah I will provided a base toolchain run list since I have both and move that to the config area. I agree with your suggestion to keep the gate, reconstruction, and geometry in one release directory rather than requiring separate paths and hashes. Those were mostly left over from development when I was swapping model versions and trying to prevent mismatches.

I agree with the main architecture that a standard python tool would be cleaner, I have started looking into that approach and it seems feasible. The main question so far is the python direct access to all MRD and Tank cpp objects and I also need to verify that the reconstruction outputs remain the same to this version to sus out any issues I have ran into before or new ones.

I also wanted to clarify what I wanted out of this PR as in the "beta release" idea, not the final production and implementation. Its been running successfully in the apptainer environment and grid tests but most of the testing has been limited to what Ive been able to check myself and a little help by christian on the grid side. I wanted to get this into the collaboration as others could test the reconstruction as we move forward to recovering events and data out of the complicated geometry of dead paddles and floating pmts. In a sense get it out of the vacuum state of just me.

Would you be open to making the current version available as an experimental opt-in tool while I work on the native python implementation? I'm not attached to the daemon architecture, but I'd rather not delay a broader testing of the model while rewriting the interface. If you'd prefer to keep it off this branch until then I understand that too.

[marc1uk]

And yes, portions of the code review and documentation were AI-assisted. ... I should of went back over it after I asked it to clean up my own language as sometimes I talk very direct and roughly.

Please, don't ask the AI to clean it up. 🙈 Documentation written by an AI doesn't read like a how-to, it reads like a Terms and Conditions of Use. It's excessively verbose, full of jargon, and while technically correct, it is not human friendly to read. I'm quite sure anything you write in your own words will be much more understandable.

Having the json interface made it esier to isolate errors and verify what was actually being passed to the models.

That .. surprises me. Using the standard Tooling you should just be able to do a BoostStore.Get to get the data in a variable and print it. You can do this on both c++ and python side. I don't see how wrapping the whole lot into a custom JSON package to transfer across an interprocess pipe would make that simpler, it seems like it just introduces a whole lot of extra complexity to debug.

The main question so far is the python direct access to all MRD and Tank cpp objects

Python Tools have access to the DataModel - anything in it, you can pull out. That said, the python base Tool class only loads DataModel.h and libDataModel.so, so if you need classes in MrdTrackLib such as cMRDTrack, you'll probably need to add equivalent lines in your Python Tool:

cppyy.add_include_path('ToolDAQ/MrdTrackLib/include')
cppyy.include('MRDSubEventClass.hh')  # and maybe others
cppyy.load_library('libFindMrdTracks.so')

For the Genie segfault, the wcsim test completed ToolAnalysis finalization for ~2000 part files so far and wrote the expected Root output before the crash, I think its just my version of rewieghtgenie doesnt like to close itself that well, I have tested it just with the base tools like FindMrdTracks and simplereco and got the same closure error.

🙈 Not ideal, but if it's not a bug introduced in this PR it would be unreasonable to expect you to be responsible for fixing it. Thanks for checking. I vaguely recall others mentioning similar things, if you haven't already please ask around (e.g. on the slack #all_software) and see if anyone else knows anything.

As for releasing this as a 'beta testing' thing... merging into main is generally not the way to do that. As you say, people can check out your branch if they want to run these Tools - they could even merge your branch into their preferred one if they need your new features on other existing updates. Merging into main a whole bunch of code that we know will all get replaced in the near future seems like overhead. I also wonder if you're being optimistic about the amount of engagement you'll get... ANNIE tends to have barely enough people for one person to work on each feature. Ask around and see if there actually are people who will run this, if it's in the main branch. Otherwise, if it's just you... there's not much reason to push for PR.

At a minimum, we would still need to address the updated python packages and new container.

[marc1uk]

I notice in your slides you say FindMrdTracks collapses near 1-4 tubes. This is a known limitation of the CA algorithm, and is meant to be addressed by it's companion Tool, TrackCombiner. Basically if there's only hits in a few MRD tubes, that's not really enough information by itself (or at least, it was considered that a few hits in a close timespan wasn't sufficient). The TrackCombiner Tool takes the tank reconstruction and uses it to do a more restrictive scope search, looking for hits in the front layers of the MRD near to where the tank track would land if projected out of the tank. (Maybe this is a more equivalent comparison to the base cMRD, whereas without it may be more similar to the no-Tank bypass?).
Sadly I don't think it's ever seen widespread use (if any), but it would be interesting to add it into the mix and see what difference it makes.
It may need a bit of reworking and tuning if the tank reconstruction has changed since it was last updated 6-7 years ago.

The fact that the track length residual distribution from the FMT is centred around -20cm seems suspect to me. Is this the length of the track in the MRD only? How was this measured?

i'm also a bit curious how in slide 16 you have a plot of cMRD Tank Start Vertex residual. How would an MRD track reconstruction algorithm determine the start of the track in the tank? 🤔

[dylondef]

Hmm, I was unaware of the track combiner, there was limited documentation provided when I was first ramping up on ANNIE, but also was just something to point out in that slide as a weakness I saw and tried to make up in cMrd.

I think to speak to that as well as your question of the tank vertex, with out forcing you through all the other documentation of this tool design comes down to one fact that FMT failed on the physical mrd having 20+ dead pmts since 2022. Avenues were chased down, but ultimately it came down to using the tank as a psudo- first layer to the mrd which then became looping rebuild logic and then the floating pmts didnt help so it led me to build this ML. To save you some time sifting through all my notes a good sum up is slide 3 of this presentation https://annie-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=6582 where a 3d render of the dead paddles are shown.

Its an mrd reco tool forsure, and has a tank-bypass switch for when theres not enough tank info, but that wasnt necessarily the problem being tackled. It was that with the hole in the mrd was losing ~ 45% of tracks FMT couldnt reconstitute, this was tested by taking wcsim samples, running the toolchain on it up to FMT and then rerunning it but with timeclustering not giving the specific ~20 paddle hit info into the clusters and to FMT.

For the -20 cm track length residual , For the MRD track length, yes, that residual is specifically for the track inside the MRD. I went back through the code I used to generate those plots, and both FMT and cMRD were compared against the same MC truth quantity (trueTrackLengthInMRD). The residual is simply reconstructed minus true track length, with no additional shifts applied to either distribution.

The difference is how the two methods rebuild the length. FMT calculates the straight-line distance between its reconstructed MRD start and stop positions, while cMRD has a dedicated ML regression output for the MRD track length.

My understanding of diving through FMT and also thesis of ANNIE is that this -20 cm comes from two things mixed. The first is the way FMT measures which is a straight line stop to start, this will lose the nuaince of the stopping muons curvature as it loses energy and is more susceptible to small angle scatters, since its comparing it to the wcsim output these are present, Julie He's thesis documented this as an issue to address when they were trying to build MuonFitter, the spiritual predecessor to cMrd. Second, FMT doesnt get the added context of the tank info when present, and is limited to the steel layer or scint paddle separation which is ~ 13 - 15.5 cm depending on what layer gap were talking about, so ultimate there is a hard line limitation on both to the physical detector. Which in itself if understood wasnt an issue I think, and not the selling point difference of FMT to cMRD Its more of the number of tracks recovered through the dead zone region of the mrd.

Thinking about it, I think your right at your collaboration point. After showing the recover ability there is desire for it, but I think the main priority is to get the first gen cross-section papers out and this will not be apart of it as far as I can tell, so I can take your notes from here and go and put it into a rebuild and if people want to use it in the mean time they can look at my fork and I can guide them from there.
Thanks for the time and feedback

[marc1uk]

Interesting slides. I think it would be easiser to visualise what impact the deadzone had on FMT if it was actually in MRD paddle-space representation, rather than electronics crate-space.

This way you can see where those dead PMTs would put holes in the tracks. FMT does permit gaps of one layer, so hits in layer 1 and 3 could still be reconstructed even without a hit in layer 2 due to a dead PMT.

It would also be interesting to see some examples of tracks that cMRD reconstructs but FMT doesn't, along with what the corresponding true particle path looked like.

For the track length reconstruction, FMT does reconstruct the path of the particle as a series of cells (from layer-to-layer), so you could feasibly add the length of those together for a better reconstruction of track length that tracks particle path. I wouldn't have expected it to add up to much though. The limitation in terms of track quantization is true however you do it. Alas I can't find my old plots of true vs reconstructed length to see what I got at the time, it was a loooong time ago now. 👴
In any case I'm not saying you need to investigate all this stuff, as the person who wrote FMT i'm just curious. :)

[dylondef]

Oh yeah that was my first chase down back last year, Bob suggested just to see if I could change the gap reconstruction when it does the cells to be able to jump at least one more layer if it needed to, but then that was locked down in a place I couldnt edit on the gvpms or at least in apptainer, I reached out to andy to see if some one could test it out but that didnt work out.

I think I have some examples of what gets recovered through the hole, I think its less of the number of failed paddles and more of their concentration into the same quadrant, if you do the scrub out before and after timeclustering makes the clusters you still see a loss of 30-40% of events compared to if you allow all paddles to in a working state. The chase down to this was a mix of Yue and James trying to compare data to MC, and getting a lopsidedness to the angles, or sides that we were able to reconstruct. Going and mapping the broken or zombie channels to MC helped us at least figure out why it was behaving that way. Its even with the same cuts of needed layers hit at least you get the loss, I think if I remember it right we were okay at about 5 dead of the deadzone, but when we exceeded that amount in a localized area it started falling off in number of tracks, and there was also a slight change in the direction of some tracks, I have another slide deck showing about 3-5% of tracks will change their angle compared to the beam axis, which caused some issues with simple reco reconstructing tank axis outside our FV and not getting used.

Def became a failure of detector maintenance, but I was only able to swap out to some new PMTS for the MRD this last September, would be interesting to see if you upped the allowed gap distance if that would of recovered some tracks.

[dylondef]

Alright, so do we want to end this pull request, ill redo the phaseIItreemaker, encapsulating the cMRD and then ill revamp the cmrdml.cpp into a a .py remove all the json logic and checks, doing that the cpp should shrink alot. but the runtime scripts will be harder, cmrd_v5_features.py I can maybe trim a bit here and there but that one will probably stay around 1000 lines, cmrd_runtime.py on the other hand has alot more of that redundent logic that we can throw out of a finall-ish product if we trust all the other debugging done so far on it. unsure how much that will cut down until i do it tho. Unsure on parity_check might be about where its at as well

[marc1uk]

Thanks for the info.
Yeah i think that's a good way to proceed. Line count isn't the only metric of good code, of course, but as with prose if you can say the same thing in fewer words, it usually improves the result. Keeping to the standard python Tool interface would be a major improvement though.
Thanks for the work so far, looking forward to seeing the rework. 👍