Bsm trigger dev/refactor PR for update to ProtoDUNEBSMWindow TAMaker by CiaranH1997 · Pull Request #105 · DUNE-DAQ/triggeralgs

CiaranH1997 · 2026-06-21T15:01:04Z

Description

If full description and testing details are included on a parent issue, please link to that here.
See issue # for details

Otherwise, please include a summary of the change and which issue is fixed (if any).
Include relevant motivation and context, including a target environment and dunedaq version if known.
Also list any dependencies that are required for this change.
Addresses issue #

Edits the ProtoDUNEBSMWindow TAMaker code only. Updates the XGBoost model for PD-HD triggering and adds a new model for PD-VD triggering. The TAMaker now only uses the XGBoost model for the collection plane TPs. For the induction planes it defaults to the sum of charge threshold in a window, just like ADCSW TAMaker. There are therefore now separate ADC thresholds for collection and induction planes. The collection plane thresholds are fixed values based on XGBoost model training. (200k for both models) and the induction plane ADC threshold is user configurable.

Please also include instructions for how a reviewer can test your changes.

I have run this algorithm in the replay app and had steady performance. I also ran dunedaq_integtest_bundle.sh and did not come across any issues. The build is successful.

Type of change

Documentation (non-breaking change that adds or improves the documentation)
New feature or enhancement (non-breaking change which adds functionality)
Optimization (non-breaking change that improves code/performance)
Bug fix (non-breaking change which fixes an issue)
Breaking change (whatever its nature)

Testing checklist

Unit tests pass (e.g. dbt-build --unittest)
Minimal system quicktest passes (pytest -s minimal_system_quick_test.py)
Full set of integration tests pass (dunedaq_integtest_bundle.sh)
Python tests pass if applicable (e.g. python -m pytest)
Pre-commit hooks run successfully if applicable (e.g. pre-commit run --all-files)

Comments here on the testing

Further checks

Code is commented where needed, particularly in hard-to-understand areas
Code style is correct (dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)
If applicable, new tests have been added or an issue has been opened to tackle that in the future.
(Indicate issue here: # (issue))

Merge branch 'develop' of https://github.com/DUNE-DAQ/triggeralgs into bsm_trigger_dev/refactor

…hm code.

MRiganSUSX

Thanks @CiaranH1997,
Several changes are suggested.

CiaranH1997 · 2026-06-24T11:45:53Z

Made all changes requested and tested TAMaker again in replay mode. Stable trigger rate at 1Hz with BDE only upstream files from PD-VD.

MRiganSUSX

Thanks for the updates @CiaranH1997.

I have few last suggestions below. Please also see a comment on one of the previous suggestions.

After these are addressed I'd go over testing again and we should be ready to proceed.

… default threshold to match appmodel default.

MRiganSUSX

Thank you for the updates.

There are very few small suggestions left.

MRiganSUSX · 2026-07-22T12:19:02Z

  // If we are in PD-VD, set boolean to true to enable effective channel mapping
  if (m_channel_map_name == "PD2VDTPCChannelMap" || m_channel_map_name == "PD2VDBottomTPCChannelMap" ||
      m_channel_map_name == "PD2VDTopTPCChannelMap") {
    m_pdvd_map = true;
  } else { // else we are in PD-HD and we use true channel mapping
    m_pdvd_map = false;
  }


You have extended this block to recongnize the new map names (Top,Bottom) but this was not propagated to https://github.com/DUNE-DAQ/triggeralgs/blob/bsm_trigger_dev/refactor/src/ProtoDUNEBSMWindow/DetectorPlaneMap.cpp#L8-L11.

It works 'by accident' via the else fallthrough. I suggest extending DetectorPlaneMap.cpp logic to deal with these cases as well:

if (channel_map_name == "PD2HDTPCChannelMap") plane_map = &pdhd_plane_map; else if (channel_map_name == "PD2VDTPCChannelMap" || channel_map_name == "PD2VDBottomTPCChannelMap" || channel_map_name == "PD2VDTopTPCChannelMap") plane_map = &pdvd_plane_map; else throw std::invalid_argument("Unknown channel map: " + channel_map_name);

Implemented this.

MRiganSUSX · 2026-07-22T12:35:10Z

If there is nothing, I would suggest completely removing this and changing the .hpp to a default constructor (also the nothing can be misleading, there is plenty being cleaned).

The suggestion for the .hpp is in another comment.

Yes, agreed. I have removed this and updated the .hpp.

MRiganSUSX · 2026-07-22T12:39:44Z

Suggest changing this to

~TAMakerProtoDUNEBSMWindowAlgorithm() override = default;

and removing the explicit .cpp definition (another comment).

Made this suggested change.

MRiganSUSX · 2026-07-22T12:55:53Z

    // If we are in PD-VD use 'effective' channel mapping for CRPs
+    // (but only for collection plane)
+    if (plane != 2) m_pdvd_map = false;


This m_pdvd_map usage is quite confusing.

For completeness:

the declaration is: https://github.com/DUNE-DAQ/triggeralgs/blob/bsm_trigger_dev/refactor/include/triggeralgs/ProtoDUNEBSMWindow/TAMakerProtoDUNEBSMWindowAlgorithm.hpp#L87

first set: https://github.com/DUNE-DAQ/triggeralgs/blob/bsm_trigger_dev/refactor/src/TAMakerProtoDUNEBSMWindowAlgorithm.cpp#L135-L137

mutation (where we are now): https://github.com/DUNE-DAQ/triggeralgs/blob/bsm_trigger_dev/refactor/src/TAMakerProtoDUNEBSMWindowAlgorithm.cpp#L51

The meaning of the var changes throughout. Initially, it means pd-vd detector, then in the process() it now means collection plane. Since the PR also introduces m_collection_plane, I suggest decoupling the two variables instead of overlapping meanings.

Suggestion is something like this:

// configure() — unchanged, m_pdvd_map set here and never touched again if (m_channel_map_name == "PD2VDTPCChannelMap" || m_channel_map_name == "PD2VDBottomTPCChannelMap" || m_channel_map_name == "PD2VDTopTPCChannelMap") { m_pdvd_map = true; } // process(), is_empty() branch — remove the mutation, use m_collection_plane only if (plane > 1) m_collection_plane = true; // REMOVE: if (plane != 2) m_pdvd_map = false; // guard effective channel mapper with both conditions — reads naturally: // "PD-VD detector AND this instance is on the collection plane" if (m_pdvd_map && m_collection_plane) { m_pdvd_eff_channel_mapper = std::make_unique<PDVDEffectiveChannelMap>(...); m_first_channel = m_pdvd_eff_channel_mapper->remapCollectionPlaneChannel(m_first_channel); m_chan_bin_length = m_pdvd_eff_channel_mapper->getNEffectiveChannels() / m_num_chanbins; } else { m_chan_bin_length = n_channels_on_plane / m_num_chanbins; }

Yes agreed. I have made this change. So now "if (plane != 2) m_pdvd_map = false;" has been removed and I guarded the use of the effective channel mapper for PD-VD as suggested.

MRiganSUSX · 2026-07-22T13:06:37Z


      m_first_channel = m_pdvd_eff_channel_mapper->remapCollectionPlaneChannel(m_first_channel);
-      m_last_channel = m_first_channel + m_pdvd_eff_channel_mapper->getNEffectiveChannels();
      m_chan_bin_length = m_pdvd_eff_channel_mapper->getNEffectiveChannels() / m_num_chanbins;


There is a very subtle 'problem' here. Because this is using integer division, for PDVD this will have a reminder (584/10=58 r4). Therefore the TPs in the last 4 highest effective channels are never binned. Is this consistent with how the model was trained or are these 4 chans expected to be live ?

Thank you for spotting this. The model is trained with a binning that incorporates channels 581/2/3/4 into the 10th channel bin, so bin ID 9. This is not the case currently in the code. I have changed the channel bin ID calculation in the ProtoDUNEBSMWindow::bin_window() function to be:

size_t channel_bin = static_cast<size_t>(((temp_tp_channel - first_channel) * num_chan_bins) / n_channels_on_plane);

This method of calculation should now append effective channel IDs 581/2/3/4 to the 10th channel bin and match exactly the method of binning used to create the model training data.

This required some extra changes to include the number of channels (or effective channels) on the plane as an argument of bin_window(...), instead of the channel bin width.

MRiganSUSX · 2026-07-22T13:15:55Z

Since the bdt_threshold is not modified when used (https://github.com/DUNE-DAQ/triggeralgs/blob/bsm_trigger_dev/refactor/src/ProtoDUNEBSMWindow/CompiledModelInterface.cpp#L27-L34) suggest just passing as value

bool Classify(const float* result, float bdt_threshold);

MRiganSUSX · 2026-07-22T13:16:55Z

As mentioned before, suggest chaning to:

bool Classify(const float* result, float bdt_threshold) { return result[0] > bdt_threshold; }

(passing bdt_threshold by value, since it is not modified).

MRiganSUSX · 2026-07-22T13:19:46Z

This is not problematic at the moment, but to make sure future code that could change the order of inits later on, I suggest being safe and changing this to:

timestamp_t m_last_pred_time = 0;

I have changed this to initialise this parameter to 0.

…But had to change the way channel bin is calculated

CiaranH1997 · 2026-07-30T21:22:08Z

I have implemented all of the above comments and re-run the replay app again. The replay monitored in grafana looks good. Physics performance of this trigger algorithm in LArSoft looks good using the set of models implemented in this TAMaker.

Ciaran Mark Hasnip and others added 5 commits March 20, 2026 15:46

Update to include new PD-HD and PD-VD models.

f10ce10

Update ProtoDUNEBSMWindow branch to build with develop

a231e42

Merge branch 'develop' of https://github.com/DUNE-DAQ/triggeralgs into bsm_trigger_dev/refactor

Comment out references to meant TOT and peak adc calculation.

ca7ec13

Update PD-VD and PD-HD models to latest versions and clean up algorit…

eb30d6f

…hm code.

Compile error fix and some tidying.

bcd569b

CiaranH1997 requested review from MRiganSUSX and alessandrothea June 21, 2026 15:01

CiaranH1997 self-assigned this Jun 21, 2026

MRiganSUSX requested changes Jun 23, 2026

View reviewed changes

Implemented PR comments from Michal.

c4bf56d

CiaranH1997 requested a review from MRiganSUSX June 25, 2026 09:29

MRiganSUSX requested changes Jun 25, 2026

View reviewed changes

Comment thread include/triggeralgs/ProtoDUNEBSMWindow/TAMakerProtoDUNEBSMWindowAlgorithm.hpp Outdated

Comment thread include/triggeralgs/ProtoDUNEBSMWindow/TAMakerProtoDUNEBSMWindowAlgorithm.hpp Outdated

Addressing final PR comments. Remove m_last_channel from .hpp and set…

cdf5a28

… default threshold to match appmodel default.

CiaranH1997 requested a review from MRiganSUSX June 26, 2026 06:54

MRiganSUSX requested changes Jul 22, 2026

View reviewed changes

Update to include Michal's comments. Mostly straightforward changes. …

9423452

…But had to change the way channel bin is calculated

CiaranH1997 requested a review from MRiganSUSX July 30, 2026 21:20

Uh oh!

Conversation

CiaranH1997 commented Jun 21, 2026

Description

Type of change

Testing checklist

Further checks

Uh oh!

MRiganSUSX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CiaranH1997 commented Jun 24, 2026

Uh oh!

MRiganSUSX left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MRiganSUSX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CiaranH1997 commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MRiganSUSX left a comment •

edited

Loading