CHG: MStripHit cleanup by zoglauer · Pull Request #171 · cositools/nuclearizer

zoglauer · 2026-06-29T00:23:10Z

I did the same cleanup for MStripHit.

But here are a few more old member functions around.

I would like to remove:
//! DEPRECATED: Set whether the strip is on the low voltage side using IsLowVoltageStrip instead
void IsXStrip(bool LowVoltageSide) { m_ReadOutElement->IsLowVoltageStrip(LowVoltageSide); }
//! DEPRECATED: Return whether the strip is on the low voltage side using IsLowVoltageStrip instead
bool IsXStrip() const { return m_ReadOutElement->IsLowVoltageStrip(); }
--> That is now IsLowVoltageStrip(..) REMOVE?

//! Set the timing in nanoseconds
void SetTiming(double Timing) { m_Timing = Timing; }
//! Return the timing in nanoseconds
double GetTiming() const { return m_Timing; }

//! Set the timing resolution
void SetTimingResolution(double TimingResolution) { m_TimingResolution = TimingResolution; }
//! Return the timing resolution
double GetTimingResolution() const { return m_TimingResolution; }
--> Has been replaced by the same function with name TAC instead of timing. REMOVE?

//! Set the temperature of the relevant preamp (in degrees C)
void SetPreampTemp(double PreampTemp) { m_PreampTemp = PreampTemp; }
//! Return the temperature of the relevant preamp (in degrees C)
double GetPreampTemp() const { return m_PreampTemp; }
--> What about that - we definitely have no preamps, but maybe some temperature dependence? But do we plan to store the temperature here?

fhagemann · 2026-06-29T00:43:37Z

I'm not sure that TAC and Timing mean the same thing (also I differentiate between the two in the DEE).
Based on the short description in #11 where m_TAC was introduced, I believe m_TAC is the raw TAC value in ADC units and Timing is the calibated TAC value in nanoseconds.
While I agree, that we might not need both TACResolution and TimingResolution, I would still vote for keeping both m_Timing and m_TAC.

fhagemann · 2026-06-29T00:49:44Z

Also, when I do data/sim comparisons, I usually write both TAC (raw TAC value) and Timing (calibrated TAC value) to an ROA file, and they have different values.

The only problem here is that we are inconsistent with types between nuclearizer and MEGAlib:
m_Timing here in nuclearizer is of type double (which for nanoseconds makes sense I would say), whereas in MEGAlib Timing is defined as unsigned int -- more consistent with this actually being an ADC value which it isn't in nuclearizer anymore (https://github.com/zoglauer/megalib/blob/develop-cosi/src/fretalon/base/inc/MReadOutDataTiming.h). This requires me to change Timing in MEGAlib on my laptop from unsigned int to double to read/write ROA files.

fhagemann · 2026-06-29T00:52:04Z

@zoglauer related to this, I would like to resurface the discussion we had back in #120, that addressed exactly this inconsistency between TAC and Timing.

fhagemann · 2026-06-29T00:58:44Z

+  //! DEPRECATED: Set whether the strip is on the low voltage side using IsLowVoltageStrip instead
+  void IsXStrip(bool LowVoltageSide) { m_ReadOutElement->IsLowVoltageStrip(LowVoltageSide); }
+  //! DEPRECATED: Return whether the strip is on the low voltage side using IsLowVoltageStrip instead
  bool IsXStrip() const { return m_ReadOutElement->IsLowVoltageStrip(); }


This can go, as long as it's not used anywhere else in nuclearizer (I see some occurrences in the old DEE files, in apps/ThresholdDeterminator.cxx, and in src/MModuleLoaderMeasurementsROA.cxx -- the latter would need to be updated then).

I made an issue since a few other parts in the code still use it.

fhagemann · 2026-06-29T00:59:43Z

+  //! Set the uncorrected ADC units of the strip (before common-mode correction)
  void SetUncorrectedADCUnits(double UncorrectedADCUnits) { m_UncorrectedADCUnits = UncorrectedADCUnits; }
-  //! Return the uncorrected ADCUnits of the strip (before common-mode correction)
+  //! Return the uncorrected ADC units of the strip (before common-mode correction)
  double GetUncorrectedADCUnits() const { return m_UncorrectedADCUnits; }

-  //! Set the ADCUnits of the strip
+  //! Set the ADC units of the strip
  void SetADCUnits(double ADCUnits) { m_ADCUnits = ADCUnits; }
-  //! Return the ADCUnits of the strip
+  //! Return the ADC units of the strip
  double GetADCUnits() const { return m_ADCUnits; }


Just from looking at this, I wouldn't know what this is (energy or timing in ADC units, before some common-mode correction?)

fhagemann · 2026-06-29T01:00:39Z

  //! Set whether the strip has triggered
  void HasTriggered(bool HasTriggered) { m_HasTriggered = HasTriggered; }
  //! Return whether the strip has triggered
  bool HasTriggered() const { return m_HasTriggered; }


"triggered" as in above slow threshold (energy triggered), or could this also be exceeding the fast threshold (timing), without exceeding the slow threshold, what we see for some peak-dip pulse shapes in non-charge-collecting neighbor strips?

I believe its the former, because I also see HasFastTiming, but maybe make this explicit in the comment.

fhagemann · 2026-06-29T01:02:36Z

+  //! Set the temperature of the relevant preamp (in degrees C)
  void SetPreampTemp(double PreampTemp) { m_PreampTemp = PreampTemp; }
-  //! Return the Temperature of the relavent preamp (in degrees C)
+  //! Return the temperature of the relevant preamp (in degrees C)
  double GetPreampTemp() const { return m_PreampTemp; }


This only seems to be used in the old DEE files, and we have an issue to keep track on how to deal with in-flight operational parameters, like temperature of the ASICs (#104 / #105)

fhagemann · 2026-06-29T01:04:50Z

+  //! Set the calibrated-timing flag
  void HasCalibratedTiming(bool CalibratedTiming) { m_HasCalibratedTiming = CalibratedTiming; }
-  //! Return a boolean indicating whether the strip timing has been calibrated;
+  //! Return whether the strip timing has been calibrated
  bool HasCalibratedTiming() const { return m_HasCalibratedTiming; }


Is this because m_Timing used to first hold uncalibrated TAC and then calibrated TAC. Or is this to flag all strips where we don't have a TAC cal available for example (guard ring for example)?

I assume this is a hold-over, as you say, but @NicoleRodriguezCavero is using this in the TAC Calibration module. I think we should be using the guard ring and fast threshold flags to determine when we do not have a valid TAC cal, but Nicole can better comment on her intention with this variable.

I still don't know how this is relates to HasFastTiming.
The comment //! Return whether the strip timing has been calibrated suggests that this flag indicates that m_Timing was calculated from m_TAC, such that we can trust whatever is stored in m_Timing (?)

fhagemann · 2026-06-29T01:08:22Z

-  //! Flags denoting the type of strip hit
+  //! True if the hit is a guard ring
  bool m_IsGuardRing;
+  //! True if the hit is a nearest neighbor


Suggested change

//! True if the hit is a nearest neighbor

//! True if the hit is a nearest neighbor hit

fhagemann · 2026-06-29T01:11:34Z

  if (WithTAC == true) {
    S<<m_TAC<<" ";
  }
  if (WithTemperature == true) {
    S<<m_PreampTemp<<" ";
  }
  if (WithEnergy == true) {
    S<<m_Energy<<" ";
  }
  if (WithTiming == true) {
    S<<m_Timing<<" ";


This is what I was referring to: we can write both (uncalibrated) m_TAC and (calibrated) m_Timing to ROA files, but currently we cannot READ the Timing because MEGAlib parses it as unsigned int where here in nuclearizer we define it as double.

fhagemann · 2026-06-29T01:11:54Z

+  //    v = Is a nearest neighbor
+  //     v = Is a guard ring


Suggested change

// v = Is a nearest neighbor

// v = Is a guard ring

// v = Is a nearest neighbor hit

// v = Is a guard ring hit

Please resolve this comment if you are not planning on incorporating it

fhagemann · 2026-06-29T01:12:11Z

+  //    v = Is a nearest neighbor
+  //     v = Is a guard ring


Suggested change

// v = Is a nearest neighbor

// v = Is a guard ring

// v = Is a nearest neighbor hit

// v = Is a guard ring hit

Please resolve this comment if you are not planning on incorporating it

fhagemann · 2026-06-29T01:15:00Z

+  // All three flags -> 7
  H.IsGuardRing(true);
  H.IsNearestNeighbor(true);
  H.HasFastTiming(true);


For the unit tests, this is fine, but I would argue that we never flag a guard ring hit also as nearest neighbor 😉

zoglauer · 2026-06-29T18:17:20Z

The TAC / timing issues is that for the balloon "timing" was TAC and thus unsigned int. And now we reused is as calibrated TAC...
OK. I changed it in MEAGlib develop-cosi to double, so that should be in alignment now.

fhagemann · 2026-06-29T18:26:58Z

One question on your change to MEGAlib develop-cosi:
zoglauer/megalib@0c2864c#r190660654

The MReadOutDataTiming::Parse line reading m_Timing is commented out. Why is that?

zoglauer · 2026-06-29T21:09:21Z

I did some updates, but we will go through them tomorrow during the calibration meeting

ckierans · 2026-06-30T03:09:45Z

+  // Energy section:

-  //! Set the ADCUnits of the strip
+  //! REVIEW: Why is this not unsigned int - that is what we measure?


Yes, this is what we measure, so unsigned int makes sense.

But is there a reason why we wouldn't want this as unsigned int for the DEE @fhagemann ? I assume we probably don't care about the small loss of accuracy from double -> int if we rounded the ADC value that is calculated in the DEE.

I mean, if we want to mimic data in the DEE, we want to convert the „uncalibrated“ energy in ADC units to an unsigned int to account for the loss in precision due to the discretization that we have intrinsically in data.

From the DEE side, changing this to unsigned int should not be an issue

ckierans · 2026-06-30T03:12:03Z

+  //! Set the uncorrected ADC units of the strip (before common-mode correction)
+  void SetUncorrectedADCUnits(double UncorrectedADCUnits) { m_UncorrectedADCUnits = UncorrectedADCUnits; }
+  //! Return the uncorrected ADC units of the strip (before common-mode correction)
+  double GetUncorrectedADCUnits() const { return m_UncorrectedADCUnits; }


Looks like a hold-over from a previous time when common-mode corrections were used. But this isn't something we currently do.

I would also argue for correcting the energy not the ADC, so perhaps we can remove these for now and then implement the temperature correction on the energy at a later time.

ckierans · 2026-06-30T03:14:35Z

+  //! Return the measured TAC value of the strip (arrival timing)
  double GetTAC() const { return m_TAC; }

+  //! REVIEW: This is not used - timing should have a resolution


Yes, I agree we can remove these

ckierans · 2026-06-30T03:15:25Z

+
+  // Timing:
+
+  //! REVIEW: Why is TAC double, when we measure unsigned ints?


Same as above for ADC. We're measuring ints, so unless we need double for the DEE (which I don't think is the case) then we should change to int.

ckierans · 2026-06-30T03:16:17Z

+
+  // Temperature:
+
+  //! REVIEW: Hold-over from balloon and balloon temperature calibration


Yes, these are old and we'll likely handle the temperature in a different way. I think you can remove these.

ckierans · 2026-06-30T03:29:02Z

  bool HasFastTiming() const { return m_HasFastTiming; }

-  //! Set the Calibrated Timing flag
+  //! REVIEW: What kind of trigger is this: above slow or above fast?


I assume this is the slow threshold since the above fast-timing flag indicates the fast threshold. I don't know if there is a good use case for this flag in the forward pipeline, and it doesn't look like it's currently used.

ckierans · 2026-06-30T03:35:38Z

+  //! Set the calibrated-timing flag
  void HasCalibratedTiming(bool CalibratedTiming) { m_HasCalibratedTiming = CalibratedTiming; }
-  //! Return a boolean indicating whether the strip timing has been calibrated;
+  //! Return whether the strip timing has been calibrated
  bool HasCalibratedTiming() const { return m_HasCalibratedTiming; }


I assume this is a hold-over, as you say, but @NicoleRodriguezCavero is using this in the TAC Calibration module. I think we should be using the guard ring and fast threshold flags to determine when we do not have a valid TAC cal, but Nicole can better comment on her intention with this variable.

ckierans · 2026-06-30T03:38:36Z

+  //! ADC units before all corrections
  double m_UncorrectedADCUnits;
-  //! ADCUnits after any correction
+  //! ADC units after any correction


I think we should just have a single ADC here and remove the Uncorrected version of the variable.

ckierans · 2026-06-30T03:40:19Z

+  //! True if the hit has fast timing
  bool m_HasFastTiming;
-  //! Flag indicating whether the hit has calibrated timing
+  //! True if the hit has calibrated timing


Not sure this is needed.

--> only reason I see would be to indicate than m_Timing doesn't store a trash value (?), but then we should check if this flag is actually set accordingly.

zoglauer · 2026-07-02T19:44:34Z

OK. I am done for the moment.
I will switch from HasCalibratedTiming, to HasTiming in the next PR, since this would change to many unrelated file for this cleanup PR.

zoglauer · 2026-07-02T19:49:08Z

Final, final changes

fhagemann · 2026-07-07T22:29:16Z

 {
  // Stream the read-out assembly in MEGAlib's ROA format
+  //
+  // WithTemperatures is currently not used, since we don't have that housekeeping info at the moment


The ROA event saver still allows to set Include temperatures, so this is just going to get ignored, but we keep the button in the GUI?

fhagemann · 2026-07-07T22:30:12Z

  if (line[0] == 'S' && line[1] == 'H') {
    unsigned int det_id, strip_id;
    int has_triggered;
-    double timing, un_adc, adc;
+    double timing, adc;
    double energy, energy_res;
    char pos_strip;
    unsigned int flags;
-    int N = sscanf(&line[3], "%u %c %u %d %lf %lf %lf %lf %lf %u",
+    int N = sscanf(&line[3], "%u %c %u %d %lf %lf %lf %lf %u",
                   &det_id, &pos_strip, &strip_id, &has_triggered,
-                   &timing, &un_adc, &adc, &energy, &energy_res, &flags);
-    if (N != 10) {
-      if (g_Verbosity >= c_Error) cout<<"Error in MStripHit::Parse: malformed SH line with "<<N<<" fields instead of 10"<<endl;
+                   &timing, &adc, &energy, &energy_res, &flags);
+    if (N != 9) {
+      if (g_Verbosity >= c_Error) cout<<"Error in MStripHit::Parse: malformed SH line with "<<N<<" fields instead of 9"<<endl;


This means all previously generated files would need to be re-generated?

fhagemann · 2026-07-07T22:34:21Z

+  //! Set the calibrated-timing flag
  void HasCalibratedTiming(bool CalibratedTiming) { m_HasCalibratedTiming = CalibratedTiming; }
-  //! Return a boolean indicating whether the strip timing has been calibrated;
+  //! Return whether the strip timing has been calibrated
  bool HasCalibratedTiming() const { return m_HasCalibratedTiming; }


I still don't know how this is relates to HasFastTiming.
The comment //! Return whether the strip timing has been calibrated suggests that this flag indicates that m_Timing was calculated from m_TAC, such that we can trust whatever is stored in m_Timing (?)

fhagemann · 2026-07-07T22:35:19Z

+  //! True if the hit has fast timing
  bool m_HasFastTiming;
-  //! Flag indicating whether the hit has calibrated timing
+  //! True if the hit has calibrated timing


--> only reason I see would be to indicate than m_Timing doesn't store a trash value (?), but then we should check if this flag is actually set accordingly.

fhagemann · 2026-07-07T22:36:49Z

+  //    v = Is a nearest neighbor
+  //     v = Is a guard ring


Please resolve this comment if you are not planning on incorporating it

fhagemann · 2026-07-07T22:36:54Z

+  //    v = Is a nearest neighbor
+  //     v = Is a guard ring


Please resolve this comment if you are not planning on incorporating it

CHG: MStripHit cleanup

6f12045

zoglauer requested review from ckierans and fhagemann June 29, 2026 00:23

fhagemann reviewed Jun 29, 2026

View reviewed changes

CHG: More updates to MStripHit

f1887ae

fhagemann added the cleanup Code cleanup label Jun 29, 2026

ckierans reviewed Jun 30, 2026

View reviewed changes

zoglauer added 2 commits July 1, 2026 12:16

MRG: develop/em

dd59e74

CHG: More cleanup

65fd787

zoglauer added 2 commits July 2, 2026 12:46

CHG: One more comment cleanup

b3304f1

CHG: Final changes

5005c60

fhagemann reviewed Jul 7, 2026

View reviewed changes

	//! True if the hit is a nearest neighbor
	//! True if the hit is a nearest neighbor hit


		// Timing:

		//! REVIEW: Why is TAC double, when we measure unsigned ints?


		// Temperature:

		//! REVIEW: Hold-over from balloon and balloon temperature calibration

Uh oh!

Conversation

zoglauer commented Jun 29, 2026

Uh oh!

fhagemann commented Jun 29, 2026

Uh oh!

fhagemann commented Jun 29, 2026

Uh oh!

fhagemann commented Jun 29, 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!

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!

zoglauer commented Jun 29, 2026

Uh oh!

fhagemann commented Jun 29, 2026

Uh oh!

zoglauer commented Jun 29, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoglauer commented Jul 2, 2026

Uh oh!

zoglauer commented Jul 2, 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