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!

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?

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

Open

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

Open

fhagemann mentioned this pull request Aug 18, 2026

Consistent use of XML tags #193

Open

ckierans reviewed Aug 19, 2026

View reviewed changes

Uh oh!

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!

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