Separate TAC calibration and cuts by NicoleRodriguezCavero · Pull Request #194 · cositools/nuclearizer

NicoleRodriguezCavero · 2026-08-14T19:40:18Z

Currently, MModuleTACCut performs the TAC calibration and applied the TAC cuts all within the AnalyzeEvent() functions. This PR:

Separates the ApplyTACCal() and ApplyTACCuts() functions and calls them in AnalyzeEvent()
No longer requires a taccut file and instead has a GUI option to set the coincidence window used in the TAC cuts
All non-GR events regardless of whether they have slow or fast timing are now calibrated to ns
Renames module from MModuleTACCut to MModuleTACCalibration
Renames XMLtags from TACcut to TACcalibration addressing part of Consistent use of XML tags #193

NicoleRodriguezCavero · 2026-08-14T19:58:21Z

The TAC cal window now looks like this:

NicoleRodriguezCavero · 2026-08-14T20:01:42Z

We can now do the TAC calibration with or without applying TAC cuts.
For non-GR hits with fast timing:

NicoleRodriguezCavero · 2026-08-14T20:07:13Z

Create separate function for TAC calibrationApplyTACCal() and call that within AnalyzeEvent()

fhagemann · 2026-08-14T20:28:18Z

The unit tests fail because the changes seem to affect the end-to-end test results on both 406-1 and 542-1.
(I updated the .cfg files locally, but I'm also getting different results now)

fhagemann · 2026-08-14T21:24:50Z

Pushing some changes to the nuclearizer config file to work with the new XML tags.

fhagemann · 2026-08-14T21:32:11Z

-    </XmlTagTACcut>
+      <ApplyTACCuts>true</ApplyTACCuts>
+      <TACCut>
+        <CoincidenceWindow>600</CoincidenceWindow>


Small comment on the 600: I know the previous taccut files had 500 in them, but that value was never used (previously, the coincidence value was always set to the hard-coded value of 600 in the code). So this setting will make sure that the unit tests are run with the same settings as before.

NicoleRodriguezCavero · 2026-08-18T19:26:09Z

These tabs were relabeled because they show the TAC distribution and energy spectra after TAC cuts (not calibrations) are applied

ckierans

Nice rework, @NicoleRodriguezCavero! I just have a few questions about the error messages, and the ordering of the modules.

Will this PR close either Issue #64 or #63 ?

ckierans · 2026-08-19T00:23:09Z

-  MModuleTACcut TACcut;
-  TACcut.SetTACCalFileName(m_TACCalFileName);
-  if (TACcut.LoadTACCalFile(m_TACCalFileName) == true) {
+  MModuleTACCalibration TACcal;


Nitpicky, but maybe use TACCal vs TACcal as the MModuleTACCalibration instance to be consistent with the capitalization of the "C" in Cal

Probably best to call this variable TACCalibration ;)

ckierans · 2026-08-19T01:32:12Z

+
+  // Set all modules, which can follow this module
+  AddSucceedingModuleType(MAssembly::c_StripPairing);
+  AddSucceedingModuleType(MAssembly::c_DepthCorrection);


The DepthCorrection module isn't actually allowable here since it has it's own dependance that StripPairing must be performed first, so I think this can be removed.

ckierans · 2026-08-19T01:37:30Z

+
+  // Set all modules, which have to be done before this module
+  AddPreceedingModuleType(MAssembly::c_EventLoader);
+  AddPreceedingModuleType(MAssembly::c_EnergyCalibration);


So now we're enforcing the EnergyCalibration to happen before the TAC Calibration? This is different than what we previously had where we left it up to the user (I think I've always done TAC first then Energy), which is probably the wrong approach! Have we confirmed there is not change of behavior with TAC Calibration -> Energy Calibration vs Energy Calibration -> TAC Calibration? I don't think it should necessarily matter, but I want to make sure we're being intentional about the order.

Honestly, this got me looking at the other Nuclearizer dependancies, and I do think we should enforce only one option for the order for TAC/Energy, so I'm happy to see this change!

I am moving this discussion to issue #207 because (1) we might need further tests with NN and module orders and (2) there will be upcoming analysis work that might want to use these modules independently. I think this should be done independently from this PR.

ckierans · 2026-08-19T01:52:56Z

+            <<" on side "<<Side
+            <<" does not have enough TAC calibration parameters - skipping event"
+            <<endl;
+        return false;


Do you mean to use return false or continue here? Return false will stop the whole Nuclearizer run if one of these conditions are met, it won't just skip the event. continue will just skip the event currently being processed. See for example lines 168 -- 174 in the MModuleEnergyCalibration for when there is no valid energy calibration for a striphit. If we want to not throw a full error and stop Nuclearizer if a TAC Calibration for a single strip is unknown, then we probably should introduce a TACCalibrationError flag, similar to the EnergyCalibrationError flag, and downgrade this to a Warning vs an Error.

I'm also wondering if it makes more sense to combine the valid calibration parameters + at least two parameters describing the calibration into one warning/error.

Take a closer look at these error messages and think about when we really need to stop Nuclearizer, and when we just need a flag for an event.

ckierans · 2026-08-19T01:54:26Z

+    if (Passed == true) {
+      ++i;
+    } else {
+      Event->RemoveStripHit(i);


Is there any reason to track the events for which we've removed striphits with the TAC cut? Does this give any indication of the quality of the event, or any information about the rest of the striphits?

I don't think we need to track events where strip hits are removed by the TAC cuts. These cuts are removing hits associated with the ASIC timing bug or chance-coincident hits which should not affect the quality of the remaining hits in the event.

NicoleRodriguezCavero · 2026-09-08T22:01:44Z

Addressing @ckierans comments (thank you!!)

Will this PR close either Issue #64 or #63 ?

This PR will resolve Issue #63, we have established that is the cut that we want. It should not resolve Issue #64 since that is more of an analysis/simulation task.

So now we're enforcing the EnergyCalibration to happen before the TAC Calibration?

I created Issue #207 to address this concern separately from the PR.

Do you mean to use return false or continue here? Return false will stop the whole Nuclearizer run if one of these conditions are met, it won't just skip the event...

I have updated the function following the EnergyCalibration conventions. Missing/invalid TAC calibrations for an individual strip now gives a TACCalibrationError flag on the event and continues instead of returning false.

Fatal errors are now only applied when the calibration file or module cannot be initialized correctly.

I also combined the "missing calibration entry" and "fewer than two calibration parameters" as Carolyn suggested.

fhagemann

Looks good!
Very minor comments from my side only

Co-authored-by: Felix Hagemann <hagemann@berkeley.edu>

ckierans

Thanks for the changes, Nicole! Things look good.

Requiring Ecal preceeding module

b4b68b4

NicoleRodriguezCavero force-pushed the taccal_separated branch from 67407bc to f3febab Compare August 14, 2026 19:48

NicoleRodriguezCavero added 2 commits August 14, 2026 12:54

Separate TAC calibration from TAC cuts

ab75942

Rename TACcut to TACcal

8ab5b2e

NicoleRodriguezCavero force-pushed the taccal_separated branch from f3febab to 8ab5b2e Compare August 14, 2026 19:54

NicoleRodriguezCavero commented Aug 14, 2026

View reviewed changes

NicoleRodriguezCavero and others added 3 commits August 14, 2026 14:14

Renaming MinNearestNeighborTiming to FLNoiseCut

5d6765d

Applying FL Noise Cut to direct hits with slow timing

f47bdb8

CHG: Update nuclearizer config file in unit tests

a7dc24a

fhagemann reviewed Aug 14, 2026

View reviewed changes

ApplyTACCal is now a function called within AnalyzeEvent

562c01f

NicoleRodriguezCavero marked this pull request as ready for review August 14, 2026 21:50

NicoleRodriguezCavero requested review from ckierans, fhagemann and zoglauer August 14, 2026 22:05

fhagemann reviewed Aug 14, 2026

View reviewed changes

Resolving Felix's comments

3e898a8

fhagemann reviewed Aug 17, 2026

View reviewed changes

Comment thread include/MAssembly.h Outdated

NicoleRodriguezCavero and others added 7 commits August 18, 2026 10:24

Changed module name to match other calibrations

9bb472f

Also updating GUI module name

6543f5a

Changed TACcal to TACCalibration

96fe366

Changing GUI spectra window labels to TAC cuts

d8a5c90

Updating Xml tag

69bf6e4

Updating module type name

3969b29

CHG: Update nuclearizer config file in unit tests

642243f

NicoleRodriguezCavero commented Aug 18, 2026

View reviewed changes

fhagemann mentioned this pull request Aug 18, 2026

CHG: Update XML tags of EnergyCalibration and DepthCalibration #197

Merged

fhagemann added the cleanup Code cleanup label Aug 18, 2026

fhagemann linked an issue Aug 18, 2026 that may be closed by this pull request

Consistent use of XML tags #193

Closed

fhagemann mentioned this pull request Aug 18, 2026

Consistent use of XML tags #193

Closed

ckierans reviewed Aug 19, 2026

View reviewed changes

NicoleRodriguezCavero linked an issue Aug 24, 2026 that may be closed by this pull request

Refine TACcut TotalOffset #64

Open

NicoleRodriguezCavero added 2 commits September 8, 2026 14:13

Resolving Carolyn's comments (1/2)

5a8f17b

Introducing TACCalibrationError flag. Carolyn's comments (2/2)

82f9b25

NicoleRodriguezCavero requested a review from ckierans September 8, 2026 23:01

fhagemann approved these changes Sep 8, 2026

View reviewed changes

Comment thread include/MModuleTACCalibration.h

Comment thread src/MGUIExpoPlotSpectrum.cxx Outdated

Comment thread include/MGUIOptionsTACCalibration.h Outdated

Resolve Felix's comments

ec54320

Co-authored-by: Felix Hagemann <hagemann@berkeley.edu>

ckierans approved these changes Sep 15, 2026

View reviewed changes

ckierans merged commit 8fab3a4 into cositools:develop/em Sep 15, 2026
1 check passed

This was referenced Sep 15, 2026

CHG: Enclose all cout statements in g_Verbosity #205

Merged

StripEnergyThresholdFinder for per-strip slow and fast threshold extraction with diagnostics #166

Open

fhagemann mentioned this pull request Sep 26, 2026

Trapping correction module and app #184

Open

ckierans mentioned this pull request Oct 2, 2026

Refine TACcut coincidence window #63

Closed

Conversation

NicoleRodriguezCavero commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicoleRodriguezCavero commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhagemann commented Aug 14, 2026

Uh oh!

fhagemann commented Aug 14, 2026

Uh oh!

fhagemann Aug 14, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

NicoleRodriguezCavero commented Sep 8, 2026

Uh oh!

fhagemann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ckierans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NicoleRodriguezCavero commented Aug 14, 2026 •

edited

Loading

NicoleRodriguezCavero commented Aug 14, 2026 •

edited

Loading

fhagemann Aug 14, 2026 •

edited

Loading