Skip to content

Conversation

@PetrilloAtWork
Copy link
Member

@PetrilloAtWork PetrilloAtWork commented Nov 19, 2025


Note to SBN release managers

This pull request is targeted to a user branch and need not be taken care by the release management.
There is a related request #871 for the shared code (develop) which instead will be under their umbrella.


These modules are special in that they compose the list of input tags from a base tag and an instance name from a list. The simplistic tag creation logic did not account for base input tags with process names.

Now the base tag can be specified as a full input tag and an exception is raised if there is a conflict between the input tag instance and the instance in the list (if both are not empty — I expect the instance name of the input tag to be always empty; otherwise, a new protocol needs to be developed). Both module label and process name are correctly propagated.

The fix has been applied to all modules known to suffer from the issue; several of them are not commonly used, and a couple are actually deprecated and should be removed.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates trigger simulation modules to support input tags with process names, not just module labels. Previously, these modules could only accept a base module label and would compose InputTags by adding threshold instance names. Now they accept full art::InputTag objects, correctly preserving process names when creating derived tags.

Key changes:

  • Changed TriggerGatesTag parameter type from std::string to art::InputTag across multiple trigger modules
  • Added makeTag/makeInputTag helper functions that properly construct InputTags with conflict detection
  • Removed conditional compilation block in TriggerSimulationOnGates for cleaner code

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
TriggerSimulationOnGates_module.cc Updated to accept InputTag parameter, added makeTag helper, removed conditional compilation
TriggerEfficiencyPlots_module.cc Updated to accept InputTag parameter and added makeTag helper
TriggerEfficiencyPlotsBase.h/.cxx Updated to accept InputTag parameter and added makeInputTag helper
SlidingWindowTrigger_module.cc Updated to accept InputTag parameter and added makeTag helper
SlidingWindowTriggerSimulation_module.cc Updated to accept InputTag parameter and added makeTag helper
MajorityTriggerSimulation_module.cc Updated to accept InputTag parameter and added makeTag helper
LVDSgates_module.cc Updated makeTag/makeOutputInstanceName to handle InputTag parameters with additional validation
CMakeLists.txt Added sbnalg dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +895 to 896
: inputTag.label() + inputTag.instance()
;
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The logic has changed from the original. Previously, when inputTag.label() didn't match defModule, it would return either inputTag.label() if the instance was empty, or inputTag.label() + inputTag.instance() if the instance was not empty. Now it always returns inputTag.label() + inputTag.instance(), which means when instance is empty, it will just return the label. While this may be the intended behavior, this change in logic should be carefully verified to ensure it doesn't break existing functionality, especially since the concatenation logic has been simplified.

Suggested change
: inputTag.label() + inputTag.instance()
;
: (inputTag.instance().empty()
? inputTag.label()
: inputTag.label() + inputTag.instance());

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

The code appears to me fully equivalent, still returning inputTag.label() + inputTag.instance() if the instance is not empty, and inputTag.label() if the instance is empty (in which case it is the same as inputTag.label() + inputTag.instance()).

Good catches on names and strings.

Co-authored-by: Copilot <[email protected]>
@PetrilloAtWork PetrilloAtWork changed the title Trigger simulation modules now support input tags with process name Trigger simulation modules now support input tags with process name [private] Nov 19, 2025
Copy link
Member

@mvicenzi mvicenzi left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@mvicenzi mvicenzi merged commit ab9ecf2 into SBNSoftware:feature/mvicenzi_repro_step1 Nov 19, 2025
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