Implementing NN into Strip Pairing by julianmgerber · Pull Request #183 · cositools/nuclearizer

julianmgerber · 2026-07-24T01:10:30Z

Previously, there was nothing about NN in the strip pairing, so if someone tried the strip pairing with NN included then it'd try to group NN + triggered strips together and add in all the energy on the NN strips. Perhaps eventually we'll want to add in some of the NN energy, but for now I thought it made sense to keep them separate.

Changes:

Separate out NN/triggered strips so that only the triggered strips are actually paired and grouped into hits
After the pairing, NN strips are linked to their neighboring hit through a new function: AddNearestNeighborStripHit
If it's ambiguous which hit a NN strip hit should be associated with (like if there are two hits separated by only one strip), then the NN strip is linked to both

Questions:

Should there be a way to read out the NN strip hits into a .dat file (since right now, only the actual triggered SH making up a hit will be read out)?
- Update 7/28: Reading out NN strip hits to .dat file as "NN" rather than "SH"
Should there be a separate flag for those ambiguous NN strip hits that are linked to two different hits?
- Update 7/28: New IsAmbiguousNearestNeighbor flag added to MStripHit class to flag NNs that are linked to multiple hits

CHG: Speed optimizations

Cherry-picked this commit from PR148 to apply seperately

…rips

…arestneighborstriphit" function to MHit class

fieldrog · 2026-07-28T20:19:51Z

@julianmgerber I woud definitely like to have a flag for NN hits ambiguously attached to two different hits !

julianmgerber · 2026-07-28T22:09:42Z

@julianmgerber I woud definitely like to have a flag for NN hits ambiguously attached to two different hits !

@fieldrog Just added the new flag, IsAmbiguousNearestNeighbor to MStripHit!

fhagemann · 2026-07-28T23:41:40Z

+    <<m_ReadOutElement->GetDetectorID()<<" "
+    <<((m_ReadOutElement->IsLowVoltageStrip() == true) ? "l" : "h")<<" "
+    <<m_ReadOutElement->GetStripID()<<" "
+    <<m_HasTriggered<<" "


Just wondering: is m_HasTriggered correlated with the strip being a NN?
Maybe this here might be redundant then, as we can "reconstruct" NN from SH that did not trigger.

Andreas has removed the m_HasTriggered flag in PR #171 .

m_HasTriggered seems to not have been removed in #171.

From what I can tell, m_HasTriggered isn't actually assigned anywhere. Like in the .dat file output, all the strip hits (triggered or otherwise) just have 0 for the HasTriggered parameter.

Is this when you read data from HDF5?
Maybe, the HDF5 reader module does not assign values to m_HasTriggered? (but should?)

Yes, looks like the HDF5 reader never assigns a m_HasTriggered value:

nuclearizer/src/MModuleLoaderMeasurementsHDF.cxx

Lines 711 to 726 in 18e1de5

if (m_StripMap.HasReadOutID(StripID) == true) {

MStripHit* H = new MStripHit();

H->SetDetectorID(m_StripMap.GetDetectorID(StripID));

H->SetStripID(m_StripMap.GetStripNumber(StripID));

H->IsLowVoltageStrip(m_StripMap.IsLowVoltage(StripID));

H->SetADCUnits(ADCs);

H->SetTAC(TACs);

// Set boolean flags based on HitType and TimingType

H->IsGuardRing(HitType == 2);

if (H->IsGuardRing() == true) {

Event->SetGuardRingVeto(true);

}

H->IsNearestNeighbor(HitType == 1);

H->HasFastTiming(TimingType == 1);

Okay, I spoke with confidence when I didn't actually check the PR #171 code! Sorry about that.

I'm pretty sure we agreed we didn't need the HasTriggered boolean because it is completely redundant with (but opposite to) the IsNearestNeighbor flag. As far as I can tell, no one is using this flag anywhere so I'd vote for removing it entirely to avoid confusion going forward.

But that doesn't need to happen in this PR, to be clear.

I agree (both removing HasTriggered and that it doesn't have to happen in this PR).

ckierans · 2026-08-04T00:55:05Z

@julianmgerber Can you resolve the conflicts with the changes in MStripHit and then I'll review?

This reverts commit 9f7b813, reversing changes made to 2879859.

ckierans

Looks good, Julian. I think we need to be a bit more careful about being clear when the StripHit list is only triggered strips or also includes NN strip hits as well. Perhaps this can just be better comments, but we might consider changing some of the names to include "TriggeredStripHits" so there's no confusion.

ckierans · 2026-08-17T15:35:10Z


  //! Divide an event's strip hits by detector and LV/HV side
-    vector<vector<vector<MStripHit*>>> CollectStripHits(MReadOutAssembly* Event);
+    tuple<vector<vector<vector<MStripHit*>>>, bool> CollectStripHits(MReadOutAssembly* Event);


Are these now only triggered strip hits? Maybe make that clear in the comment?

Yes, I see below these are only the triggered strip hits. You should update the comment to reflect this change.

"//! Divide an event's triggered strip hits by detector and LV/HV side"

ckierans · 2026-08-17T15:36:18Z

  //! Ownership stays elsewhere
  MStripHit* GetStripHit(unsigned int i);
+  //! Return nearest neighbor strip hit i or nullptr if i is out of bounds
+  MStripHit* GetNearestNeighborStripHit(unsigned int i);


I'm wondering about any potential confusion/overlap with NN strip hits listed here vs in the StripHit list that then have the IsNearestNeightbor flag. Do we expect NN strip hits to also be listed in the standard GetStripHit list or is that now exclusively for triggered strips? If so, let's make that clear.

ckierans · 2026-08-17T15:50:39Z

      }
    }
+    else {
+      IncludingNearestNeighbors = true;


It took me a moment to understand what you're trying to do with this boolean. Maybe add a comment for this else statement: "If nearest neighbor strips are found in the event, set flag to properly account for them when strip pairing"

We have the boolean Get/SetIncludeNearestNeighbors in the MModuleLeaderMeasurementHDF module, which theoretically should be the same as this boolean here, except you're deriving it from the data, and not the GUI. I'm wondering if it will be less confusing (an potentially less prone to mistakes) to instead use that user-defined variable.

ckierans · 2026-08-17T16:08:37Z

  // Strip hits:

  //! Return the number of strip hits
  unsigned int GetNStripHits() const { return m_StripHits.size(); }


Is this now only triggered strip hits? Or both triggered and untriggered?

ckierans · 2026-08-17T16:11:19Z

+    bool AssignedDetector = false;
+    int DetectorID; // Define detector ID where hit took place
+    for (unsigned int sh = 0; sh < Event->GetHit(h)->GetNStripHits(); sh++) {
+      // Collect all the strip hits in a hit and split them by side


I assume that whenever this function is called, the Hit only has triggered strips assigned to it. So this GetNStripHits will only return the number of triggered strip hits. Make that clear in the comment?

Once you assign Nearest Neighbor strip hits to the event, do you expect GetNStripHits to count all associated strip hits, or only triggered strips?

ckierans · 2026-08-17T16:29:05Z

+    <<m_ReadOutElement->GetDetectorID()<<" "
+    <<((m_ReadOutElement->IsLowVoltageStrip() == true) ? "l" : "h")<<" "
+    <<m_ReadOutElement->GetStripID()<<" "
+    <<m_HasTriggered<<" "


Okay, I spoke with confidence when I didn't actually check the PR #171 code! Sorry about that.

I'm pretty sure we agreed we didn't need the HasTriggered boolean because it is completely redundant with (but opposite to) the IsNearestNeighbor flag. As far as I can tell, no one is using this flag anywhere so I'd vote for removing it entirely to avoid confusion going forward.

ckierans · 2026-08-17T16:29:23Z

+    <<m_ReadOutElement->GetDetectorID()<<" "
+    <<((m_ReadOutElement->IsLowVoltageStrip() == true) ? "l" : "h")<<" "
+    <<m_ReadOutElement->GetStripID()<<" "
+    <<m_HasTriggered<<" "


But that doesn't need to happen in this PR, to be clear.

zoglauer and others added 16 commits April 26, 2026 22:45

CHG: Speed optimizations

ec76532

Merge pull request #2 from zoglauer/feature/strip_pairing_speedup

fc91b21

CHG: Speed optimizations

Merge remote-tracking branch 'upstream/develop/em'

fe0a54e

Included option to save QA-flagged events

d73b58b

Cherry-picked this commit from PR148 to apply seperately

Fixed missing lines after cherry pick

1edeef3

Button layout

87c75ae

Merge remote-tracking branch 'upstream/develop/em'

a1ab750

Merge remote-tracking branch 'upstream/develop/em'

6097e47

Merge remote-tracking branch 'upstream/develop/em'

2879859

Separate out NN and triggered strips when collecting strip pairing st…

d96b0ec

…rips

commenting

633cf5d

function to collect NN strip hits

a872494

Assign Nearest Neighbor function in strip pairing. Created new "addne…

cc35e5e

…arestneighborstriphit" function to MHit class

bug fixes

14bce2e

adding variable to tell if nearest neighbors are included in strip hits

6a721e8

comment

a9fb662

julianmgerber requested a review from ckierans July 24, 2026 01:10

Julian Gerber added 5 commits July 28, 2026 14:07

Read out NN strip hits into .dat file

e0c430e

Changing how nearest neighbor is referenced

ae7d555

Adding flag for ambiguous neighbors

14330c2

forgot to initialize new variable

83a0cc1

Update variable/function names

b5243d3

Edit MStripHit parse function to account for new .dat readout

25551a4

julianmgerber marked this pull request as ready for review July 28, 2026 23:38

fhagemann reviewed Jul 28, 2026

View reviewed changes

fhagemann assigned fhagemann and parshadkp and unassigned fhagemann Aug 4, 2026

fhagemann unassigned parshadkp Aug 4, 2026

Julian Gerber added 6 commits August 4, 2026 02:26

Merge remote-tracking branch 'carolyn/qa_option'

9f7b813

Revert "Merge remote-tracking branch 'carolyn/qa_option'"

dc40418

This reverts commit 9f7b813, reversing changes made to 2879859.

Merge remote-tracking branch 'upstream/develop/em'

1fd176a

Merge branch 'main' into StripPairingNN

0dc4d82

Fixing bugs from merge

293e36e

Editing error message in MStripHit Parse

8adf152

ckierans reviewed Aug 17, 2026

View reviewed changes

fhagemann mentioned this pull request Aug 18, 2026

Add all options we can write a roa file also in the reader #66

Open

julianmgerber marked this pull request as draft August 19, 2026 00:36

	if (m_StripMap.HasReadOutID(StripID) == true) {
	MStripHit* H = new MStripHit();
	H->SetDetectorID(m_StripMap.GetDetectorID(StripID));
	H->SetStripID(m_StripMap.GetStripNumber(StripID));
	H->IsLowVoltageStrip(m_StripMap.IsLowVoltage(StripID));
	H->SetADCUnits(ADCs);
	H->SetTAC(TACs);

	// Set boolean flags based on HitType and TimingType
	H->IsGuardRing(HitType == 2);
	if (H->IsGuardRing() == true) {
	Event->SetGuardRingVeto(true);
	}

	H->IsNearestNeighbor(HitType == 1);
	H->HasFastTiming(TimingType == 1);

Uh oh!

Conversation

julianmgerber commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fieldrog commented Jul 28, 2026

Uh oh!

julianmgerber commented Jul 28, 2026

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!

ckierans commented Aug 4, 2026

Uh oh!

ckierans 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

julianmgerber commented Jul 24, 2026 •

edited

Loading