Skip to content

Refactoring & minor modifications of TriggerActivityMakerTPC module#44

Draft
kwawrows wants to merge 1 commit intodevelopfrom
kwawrows/tamaker_logic_fixes
Draft

Refactoring & minor modifications of TriggerActivityMakerTPC module#44
kwawrows wants to merge 1 commit intodevelopfrom
kwawrows/tamaker_logic_fixes

Conversation

@kwawrows
Copy link
Copy Markdown
Member

@kwawrows kwawrows commented Apr 7, 2026

This PR refactors TriggerActivityMakerTPC to fix a logic issue with readout plane handling, improve modularity, and make configuration more flexible.

  1. Fix TP grouping and plane handling
  • Old code grouped TPs by custom (TPCsetID, geo::View_t) scope type. This is fine for ProtoDUNE, but as the FD will contain a mix of single and double-sided APAs, grouping by readout_plane_id ensures that each collection TAMaker sees the same TPC drift volume.
  • Now TPs are grouped by readout::ROPID, one TAMaker per physical readout plane, avoiding merges across APA sides. This is also extracted directly from the geometry service, without introducing unnecessary abstractions.
  1. Dynamic per-ROP configuration
  • Previously the TAMaker configuration per plane was hardcoded to contain 4 planes (not true for the VD) & furthermore since plane config (algconfig_plane0–3) was keyed on View_t, plane3 was effectively unused (as views run from 0-2)
  • Hardcoded algconfig_plane{0–3} replaced with fcl-configurable:
  • active_rops: list of ROPs to process (if empty will process all that are present)
  • algconfig_ropX: config per ROP
  • Stored in std::map<unsigned int, nlohmann::json> algconfigs_ and built once in the constructor.
  • The number of ROPIDs is automatically extracted from the geometry service (as PD, FD-VD and FD-HD will have different number of CRPs/APAs and readout planes per detector element).
  • Allows selective processing, e.g., for collection-only TAMaking:
ta_algconfig_swift:
{
  window_length: 32000
  inspect_energy_threshold: 15000
  accept_energy_threshold: 55000
}

tamakerTPC_SWIFT:
{
  module_type: TriggerActivityMakerTPC
  tp_tag: "tpmakerTPCsimpleThr"
  algorithm: "TriggerActivityMakerSWIFTPlugin"
  active_rops: [2, 3]
  algconfig_rop2: @local::ta_algconfig_swift # collection rop2
  algconfig_rop3: @local::ta_algconfig_swift # collection rop3
  flush: true
}

3). Refactored and clarified module structure:

  • produce() handles pipeline orchestration only.
  • buildAlgConfigs() sets up algorithm configuration per readout plane.
  • toJson() converts FHiCL to JSON-like config.
  • buildAssociations() handles TP–TA associations.
  1. Other
  • Geometry service cached in constructor instead of per event.
  • Introduced IndexedTP to track original TP indices for associations.
  • Flush behaviour now calls alg->flush() directly with a max timestamp instead of feeding a dummy TP (which is problematic for algorithms which expect time-ordered input).
  • TP equality check simplified (detid comparison removed as it's redundant given TP channel check).
  • Channel masking uses std::binary_search (but this expects sorted input!)

@JamesJieranShen
Copy link
Copy Markdown
Member

Hi Klaudia, thank you so much for preparing this! I have a couple general comments on the list of changes you have kindly provided, before digging into the code itself:

1. Fix TP grouping and plane handling


* Old code grouped TPs by custom `(TPCsetID, geo::View_t)` scope type. This is fine for ProtoDUNE, but as the FD will contain a mix of single and double-sided APAs, grouping by readout_plane_id ensures that each collection TAMaker sees the same TPC drift volume.

* Now TPs are grouped by `readout::ROPID`, one TAMaker per physical readout plane, avoiding merges across APA sides. This is also extracted directly from the geometry service, without introducing unnecessary abstractions.

The original implementation (by Wes) was actually changed from ROPID to the current TPCSetID + View tuple indexing because we never decided if the two collection planes should be treated as the same "plane" or not in trigger algorithms. I believe the DAQ currently considers the two collection plane as one plane, but it is not obvious if it is better to splilt it in half. Therefore, the option was left open for either.

2. Dynamic per-ROP configuration


* Previously the TAMaker configuration per plane was hardcoded to contain 4 planes (not true for the VD) & furthermore since plane config (`algconfig_plane0–3`) was keyed on `View_t`, plane3 was effectively unused (as views run from 0-2)

* Hardcoded `algconfig_plane{0–3}` replaced with fcl-configurable:

* `active_rops`: list of ROPs to process (if empty will process all that are present)

* `algconfig_ropX`: config per ROP

* Stored in `std::map<unsigned int, nlohmann::json> algconfigs_` and built once in the constructor.

* The number of ROPIDs is automatically extracted from the geometry service (as PD, FD-VD and FD-HD will have different number of CRPs/APAs and readout planes per detector element).

* Allows selective processing, e.g., for collection-only TAMaking:
ta_algconfig_swift:
{
  window_length: 32000
  inspect_energy_threshold: 15000
  accept_energy_threshold: 55000
}

tamakerTPC_SWIFT:
{
  module_type: TriggerActivityMakerTPC
  tp_tag: "tpmakerTPCsimpleThr"
  algorithm: "TriggerActivityMakerSWIFTPlugin"
  active_rops: [2, 3]
  algconfig_rop2: @local::ta_algconfig_swift # collection rop2
  algconfig_rop3: @local::ta_algconfig_swift # collection rop3
  flush: true
}

This is all fine, but I do not believe that the default value logic for active_rops is implemented in this PR...? Apologies if I'm just being blind. Regardless, to clarify: I think the appropriate approach is that if active_rops is not specified, the default value should be that all ROPs are active. Should active_rops be specified as an empty array, TAMakers should be turned off for all planes.


3). Refactored and clarified module structure:

    * `produce()` handles pipeline orchestration only.

    * `buildAlgConfigs()` sets up algorithm configuration per readout plane.

    * `toJson()` converts FHiCL to JSON-like config.

    * `buildAssociations()` handles TP–TA associations.


    4. Other


    * Geometry service cached in constructor instead of per event.

    * Introduced IndexedTP to track original TP indices for associations.

    * Flush behaviour now calls `alg->flush() `directly with a max timestamp instead of feeding a dummy TP (which is problematic for algorithms which expect time-ordered input).

I'm a bit worried about this. All pre-existing TAMakers (ADCSW, etc) does not have flush implemented, as far as I can tell. Perhaps a compromise is to have the default flush of a TAMaker to be the old behavior of flushing a TP with MAX timestamp, but this is not a very elegant solution.

* TP equality check simplified (detid comparison removed as it's redundant given TP channel check).

This seems unnecessary to simplify -- if this field continues to exist in the TA struct, we should check for it for equality.

* Channel masking uses `std::binary_search `(but this expects sorted input!)

Like I said, I have not looked at the code very closely to make any comments on the actual implementation but hopefully these are useful things to think about:)

JS

@kwawrows kwawrows marked this pull request as draft April 8, 2026 11:00
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.

2 participants