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).

m_HasTriggered was replaced with (the inverse of) m_IsNearestNeighbor in #202 (which was also merged), so maybe instead of renaming this to NN, we can use the m_IsNearestNeighbor flag instead (?)

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: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.

GetStripHit from the MHit class should only be triggered strip hits. However, the GetStripHit function in MReadOutAssembly (for the whole event) would have both triggered and NN strips. I added some comments to clarify

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.

Felix pointed out that the HDF loader module isn't a required prerequisite for the strip pairing, so that boolean might not always be set (like if someone is just loading an ROA file, for instance). I moved this flag though to a different place and added some comments, so hopefully it's clearer.

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?

Should just be triggered. I added a comment to clarify.

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?

GetNStripHits should only count triggered SH since the triggered SH get added to m_StripHits while the NNs get added to m_NearestNeighborStripHits

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.

julianmgerber · 2026-09-22T01:15:25Z

@ckierans I updated the comments and variable names within the strip pairing to try and make it clearer where there's triggered versus NN strip hits.

For the streamDAT function, I left it reading out as "NN" instead of "SH" because I think it's a little helpful to immediately see all the nearest neighbors. But this is totally redundant with the "IsNearestNeighbor" flag, so I can change that if people prefer differently.

ckierans · 2026-09-22T15:46:57Z

This looks good, @julianmgerber! Thanks for adding all of those clarifying comments and more detailed variable names. It's much easier to follow. I like the "NN" in the .dat file.

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

Julian Gerber added 12 commits August 30, 2026 23:18

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

29383fe

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

6a78116

Merge branch 'main' into StripPairingNN

5a30de1

commenting

a9ba3ea

fix bug in GetNearestNeighborStripHit function

81fa180

using 'include nn' tag from HDF loader module

3f01a01

just iterating thru SH to check if there are neighbors

78f621d

commenting

0a9c722

change StripHits to TriggeredStripHits where relevant

696fee1

fix big in streamdat

15841ca

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

cd5a457

Merge branch 'main' into StripPairingNN

faabfaa

julianmgerber marked this pull request as ready for review September 22, 2026 01:08

ckierans merged commit aba2f79 into cositools:develop/em Sep 23, 2026
1 check passed

fhagemann mentioned this pull request Sep 24, 2026

Fixing bug with NN in strip pairing #218

Open

	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);

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!

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!

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!

julianmgerber Sep 22, 2026 • 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!

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!

julianmgerber commented Sep 22, 2026

Uh oh!

ckierans commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

julianmgerber commented Jul 24, 2026 •

edited

Loading

julianmgerber Sep 22, 2026 •

edited

Loading