Added flag for events with an ADC value higher than 14000. by nlopez-code · Pull Request #200 · cositools/nuclearizer

nlopez-code · 2026-08-19T15:26:13Z

Included in unit test as well
( #40 )

…n unit test as well

fhagemann

This is one of the most beautifully written PRs I've ever seen.
I have one tiny comment on your code, and will follow with instructions how to get the unit tests to pass :)

fhagemann · 2026-08-19T17:15:35Z

If you look at the test logs, you can see the following:

PASS UTNDEEStripHit (Passed tests: 40, Failed tests: 0)
UTNEndToEnd_406-1 (1/7 failed)
UTNEndToEnd_542-1 (1/7 failed)
PASS UTNHit (Passed tests: 139, Failed tests: 0)
PASS UTNModuleLoaderMeasurementsHDF (Passed tests: 118, Failed tests: 0)
PASS UTNReadOutAssembly (Passed tests: 255, Failed tests: 0)
PASS UTNStripHit (Passed tests: 145, Failed tests: 0)
PASS UTNStripMap (Passed tests: 236, Failed tests: 0)

which states that something in the end-to-end tests for detectors 406-1 and 542-1 failed.

You can run those tests on your local machine by running

make -j16
make -j16 unittests
UTNEndToEnd_406-1

Which in my case yields the following output:

FAILED: End-to-end test 406-1  <-- output file
   Description: The generated output is numerically equivalent to the reference
   Expected:    numerically equivalent files
   Output:      
      line 1340:
        expected:  QA StripPairing (Best reduced chi square is not below 25 (466.978427)) (GR Hit: Detector ID 0 and Energy 294.916417)
        test:      QA HighADC (High ADC value 15804 for Detector: 0, side: LV, strip: 20) (High ADC value 16013 for Detector: 0, side: LV, strip: 21) (High ADC value 16006 for Detector: 0, side: LV, strip: 22) (High ADC value 15449 for Detector: 0, side: LV, strip: 23) (High ADC value 15058 for Detector: 0, side: HV, strip: 53) (High ADC value 16015 for Detector: 0, side: HV, strip: 54) (High ADC value 16038 for Detector: 0, side: HV, strip: 55)

Unit test: UTNEndToEnd_406-1
Passed tests: 7
Failed tests: 1

Essentially this means that, if we run nuclearizer using the updated code in this PR, we would get a different output (which makes sense here: we're now producing additional quality flags, and they will end up appearing in the final .tra file.

Some background information:
In the unit tests, we automatically run nuclearizer using the config files in resource/unittestdata/406-1 (or 542-1 respectively) via nuclearizer -c hdf5-to-tra.nuclearizer.cfg -a and compare the output to hdf5-to-tra.reference.tra.
You're now getting the (expected) error that the newly generated .tra file does not match the reference .tra file.
To resolve this, you might need to update those reference files, such that they include the quality flags that you are adding in this PR. You can achieve this by manually running nuclearizer -c hdf5-to-tra.nuclearizer.cfg -a in both the 406-1 and 542-1 directories yourself, renaming the created output file hdf5-to-tra.tra to hdf5-to-tra.reference.tra to update the reference file, and pushing the updated reference file to this PR.

Let me know if you run into any issues tackling this, always happy to help! :)

fhagemann

Also, looks like my comment didn't go through ^^

fhagemann · 2026-08-19T18:25:01Z

+      if (g_Verbosity >= c_Warning) {
+        cout << m_XmlTag << ": Warning: High ADC value " << SH->GetADCUnits() << " for read-out element " << R << endl;
+      }
+      Event->SetHighADC_QualityFlag("High ADC value " + to_string(SH->GetADCUnits()) + " for " + R.ToString().Data());


Tiny comment when composing the QualityFlag Text: you might want to convert the ADC value to an (unsigned) int to avoid a lot of unnecessary zeros in the message:

Suggested change

Event->SetHighADC_QualityFlag("High ADC value " + to_string(SH->GetADCUnits()) + " for " + R.ToString().Data());

if (g_Verbosity >= c_Warning) {

cout << m_XmlTag << ": Warning: High ADC value " << (int) SH->GetADCUnits() << " for read-out element " << R << endl;

}

Event->SetHighADC_QualityFlag("High ADC value " + to_string((int) SH->GetADCUnits()) + " for " + R.ToString().Data());

This will be resolved by itself once we tackle #176, but we can hack this here for now ;)

Before:

QA HighADC (High ADC value 15804.000000 for Detector: 0, side: LV, strip: 20) (High ADC value 16013.000000 for Detector: 0, side: LV, strip: 21) (High ADC value 16006.000000 for Detector: 0, side: LV, strip: 22) (High ADC value 15449.000000 for Detector: 0, side: LV, strip: 23) (High ADC value 15058.000000 for Detector: 0, side: HV, strip: 53) (High ADC value 16015.000000 for Detector: 0, side: HV, strip: 54) (High ADC value 16038.000000 for Detector: 0, side: HV, strip: 55)

After:

QA HighADC (High ADC value 15804 for Detector: 0, side: LV, strip: 20) (High ADC value 16013 for Detector: 0, side: LV, strip: 21) (High ADC value 16006 for Detector: 0, side: LV, strip: 22) (High ADC value 15449 for Detector: 0, side: LV, strip: 23) (High ADC value 15058 for Detector: 0, side: HV, strip: 53) (High ADC value 16015 for Detector: 0, side: HV, strip: 54) (High ADC value 16038 for Detector: 0, side: HV, strip: 55)

ckierans

Nice work on your first Nuclearizer PR, @nlopez-code !!!

That said, I think you overdid the flags a bit :). The QualityFlag in MReadOutAssembly is probably sufficient, where as the flag in the MStripHit class is probably too much, at least in my interpretation of the purpose of those flags.

We also need to decide if we should do anything with the ADC values if they are measured >14000. Should we treat 14000 ADC truly like a software overflow bin and reassign the high ADC values to be 14000, or do we just keep it as is and treat these events with caution to be potentially filtered out in revan/mimrec? This probably needs to be investigated further with some high energy data to determine the best approach, but that of course can be a TODO.

Perhaps for now just add a comment in the code when you raise the High ADC warning that we might want to revisit how we handle the high ADC values.

ckierans · 2026-08-20T00:38:56Z

+  //! Set whether the strip has a high ADC value (close to the ADC saturation limit)
+  void HasHighADC(bool HighADC) { m_HasHighADC = HighADC; }
+  //! Return whether the strip has a high ADC value (close to the ADC saturation limit)
+  bool HasHighADC() const { return m_HasHighADC; }


I'm wondering if it's appropriate to be defining a StripHit flag for HighADC?

StripHits with the > HighADC value should be treated with caution, but I think the HighADC quality flag for an event, as you have defined it in MReadoutAssembly, might be enough?

The flags here for StripHits, such as HasFastTiming, IsGuardRing, etc, are derived from the raw data and describe the type of strip hit but not necessarily the quality of the strip hit. I think the HighADC flag is most similar to the SlowThresholdCut (other then us not removing the strip hit), so we should more closely follow that example. And if that's the case, then I think you did a lot more work then you needed to.

ckierans · 2026-08-20T00:40:27Z

-  // 0b111u
-
-  unsigned int Flags = 0b000u;
+  // Currently, 4 bits:


This gives me pause. I don't think we want HighADC to be looped in with these higher-level flags.

…le to restore from the previous commit.

…or good

fhagemann · 2026-08-20T19:41:04Z

Is this missing the updated MModuleEnergyCalibration.cxx?
In the current version of the files, we are not setting the flag anywhere (?)

fhagemann · 2026-08-20T22:02:51Z

+    if (SH->GetADCUnits() > m_HighADCThreshold) {
+      SH->HasHighADC(true);
+      if (g_Verbosity >= c_Warning) {
+        cout << m_XmlTag << ": Warning: High ADC value " << SH->GetADCUnits() << " for read-out element " << R << endl;


You might also want to add the (int) in this line:

Suggested change

cout << m_XmlTag << ": Warning: High ADC value " << SH->GetADCUnits() << " for read-out element " << R << endl;

cout << m_XmlTag << ": Warning: High ADC value " << (int) SH->GetADCUnits() << " for read-out element " << R << endl;

fhagemann · 2026-08-20T22:48:39Z

+    // Flag strip hits whose ADC value is close to the ADC saturation limit, since their
+    // calibrated energy is not trustworthy. The hit is kept, it is only marked.
+    if (SH->GetADCUnits() > m_HighADCThreshold) {
+      SH->HasHighADC(true);


~~This should be Event->HasHighADC(true);?~~

Edit: Actually, this line can completely be removed (the StripHit has no m_HasHighADC anymore), as Event->SetHighADC_QualityFlag later in the code takes care of that

Suggested change

SH->HasHighADC(true);

…his was supposed to be a beautifully written PR

fhagemann

Unit tests pass, looks good to me! :)

fhagemann · 2026-08-20T23:18:08Z

While I love the commit messages, we might want to squash them when merging ^^

Added flag for events with an ADC value higher than 14000. Included i…

31d90c2

…n unit test as well

fhagemann approved these changes Aug 19, 2026

View reviewed changes

fhagemann added the enhancement New feature or request label Aug 19, 2026

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

Handle non-linear gain/ADC overflow #40

Open

fhagemann reviewed Aug 19, 2026

View reviewed changes

ckierans reviewed Aug 20, 2026

View reviewed changes

nlopez-code added 3 commits August 20, 2026 14:56

Undo unnecessary changes for the highADC flag.

942c7db

Remove highADC flag information from strip hit. This was a missing fi…

a42c560

…le to restore from the previous commit.

Oops, that was restored to main and not develop-em. Okay, this time f…

de9c7fd

…or good

fhagemann reviewed Aug 20, 2026

View reviewed changes

Comment thread src/MModuleEnergyCalibration.cxx Outdated

nlopez-code added 3 commits August 20, 2026 17:18

Reintroduce missing changes.

b331670

This has been great git practice

1b0968f

Change to int value in message

6611bd5

fhagemann reviewed Aug 20, 2026

View reviewed changes

Updated the unit test reference files to include new flag for highADC.

4ffe394

fhagemann reviewed Aug 20, 2026

View reviewed changes

Fixed the integer issue in the QA message. I remember way back when t…

75621d5

…his was supposed to be a beautifully written PR

fhagemann approved these changes Aug 20, 2026

View reviewed changes

	cout << m_XmlTag << ": Warning: High ADC value " << SH->GetADCUnits() << " for read-out element " << R << endl;
	cout << m_XmlTag << ": Warning: High ADC value " << (int) SH->GetADCUnits() << " for read-out element " << R << endl;

Uh oh!

Conversation

nlopez-code commented Aug 19, 2026

Uh oh!

fhagemann left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fhagemann left a comment

Choose a reason for hiding this comment

Uh oh!

fhagemann Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

ckierans left a comment

Choose a reason for hiding this comment

Uh oh!

ckierans Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

ckierans Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

fhagemann commented Aug 20, 2026

Uh oh!

Uh oh!

fhagemann Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

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

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

fhagemann left a comment

Choose a reason for hiding this comment

Uh oh!

fhagemann commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fhagemann commented Aug 19, 2026 •

edited

Loading

fhagemann Aug 20, 2026 •

edited

Loading

fhagemann Aug 20, 2026 •

edited

Loading