Skip to content

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

Open
JarredMRoberts wants to merge 30 commits into
cositools:develop/emfrom
JarredMRoberts:feature/strip-threshold-finder
Open

StripEnergyThresholdFinder for per-strip slow and fast threshold extraction with diagnostics#166
JarredMRoberts wants to merge 30 commits into
cositools:develop/emfrom
JarredMRoberts:feature/strip-threshold-finder

Conversation

@JarredMRoberts

Copy link
Copy Markdown

Adds a standalone application, StripEnergyThresholdFinder, for computing per-strip slow and fast energy thresholds. Slow thresholds are determined from ADC spectra using a noise peak and trough method, while fast thresholds are determined from dt0/dt1 timing crossover. The tool reads calibrated HDF5 data via MModuleLoaderMeasurementsHDF, applies strip mapping and energy calibration from a YAML configuration, and produces ROOT diagnostic outputs (energy spectra with thresholds, dt0 vs dt1 per strip, and threshold distributions) along with CSV export files. The implementation is self-contained under apps/ and does not modify existing modules. Tested on COSI datasets with consistent threshold behavior and expected diagnostic results. Target branch is develop/em.

@JarredMRoberts

Copy link
Copy Markdown
Author

I still need to fix all of the code style issues and work on some optimizations to speed the code up a bit.

@fhagemann

Copy link
Copy Markdown

Is this different from #143 or making #143 obsolete?

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
@fhagemann
fhagemann marked this pull request as draft June 23, 2026 21:50
JarredMRoberts and others added 3 commits June 30, 2026 07:33
Co-authored-by: Felix Hagemann <hagemann@berkeley.edu>
Co-authored-by: Felix Hagemann <hagemann@berkeley.edu>
…nventions and integrate energy calibration improvements
@JarredMRoberts
JarredMRoberts marked this pull request as ready for review July 28, 2026 08:40
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jarred,
I had an extensive look at this app and finally got this to run.
I replied to all comments in this PR, and opened a separate PR onto your fork/branch, addressing some code changes to

  1. get this app running
  2. remove the helper classes by replacing them with existing nuclearizer/megalib code

Here is the PR onto your branch with detailed code changes: JarredMRoberts#1

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
@zoglauer

zoglauer commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

I second not to have an additional parser linked.
You can use the nuclearizer/megalib XML file format instead; your file then would be:

<strip-threshold-finder>
    <input>
        <data_files>
            <file>/path/to/data/file.hdf5</file>
        </data_files>
        <calibration_file>/path/to/calibration/file.ecal</calibration_file>
        <tac_calibration_file>/path/to/tac/calibration/file.csv</tac_calibration_file>
        <strip_map>/path/to/strip/map/file.map</strip_map>
    </input>

    <analysis>
        <!-- number of skipped strips with minimum statistics -->
        <min_entries>10</min_entries> 
        
        <!-- If a threshold cannot be determined set threshold to default value -->
        <fallback_threshold_keV>20</fallback_threshold_keV> 
        
        <!-- limit for locating the low-energy noise peak -->
        <!-- most noise peaks should be between 100 and 250 -->
        <!-- Worst case the ADC max should be set to ~1000 -->
        <noise_search_max_adc>1800</noise_search_max_adc>
    </analysis>
    
    <output>
        <prefix>output_file_prefix</prefix>
    </output>
</strip-threshold-finder>

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more remark: I got this app to run and got reasonable results for the fast and slow thresholds only when using a dataset taken with NN off.
We might want to always filter out NN events, for this app to also run using datasets taken with NN on.

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread .github/workflows/tests.yaml

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quick review with code-style comments, I will follow up with some more general comments

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx
Comment thread apps/StripEnergyThresholdFinder.cxx
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quick review with code-style comments, I will follow up with some more general comments

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

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general comments:

  • do you have some figures to walk us through the pedestal/peak finding algorithms, to get an idea in what cases the pedestal, flattop or peak detection loops would do their thing?
  • we might want to avoid having hard-coded values distributed throughout the file and define them as constants at the top of the file (or member variables of the threshold app class) instead.
  • In the end, we might want to generate ONE threshold file per threshold type (fast, slow, hardware), instead of splitting them into LV and HV subfiles. I believe that the forward pipeline will require a combined file in the end anyhow

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment on lines +796 to +797
// --- dt0 vs dt1 separation ---
bool is_dt1 = (TAC > 8000); // initial threshold

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ever not the case?
Also, should this hard-coded value become some member variable or be defined at the very beginning of the file, instead of very far into the code?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this in more detail: shouldn't this be:

Suggested change
// --- dt0 vs dt1 separation ---
bool is_dt1 = (TAC > 8000); // initial threshold
// --- dt0 vs dt1 separation ---
bool is_dt1 = SH->HasFastTiming();

??

@fhagemann fhagemann Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on data from HP52406-1 (Am-241 lifetime data, taken with HDFv2.2: gse_20260217T115220.hdf5):

With the current TAC > 8000 line, I get nothing in HV strip 32 (no dt1 distribution, everything seems to be classified as dt0), resulting in the fallback fast threshold:

WARNING: FAST threshold could not be determined for Det 0 Side HV Strip 32: insufficient timing populations (dt0=88441, dt1=293, dt1 fraction=0.3302%). Using fallback threshold = 34 keV.
image

Changing it to SH->HasFastTiming() results in reasonable dt0 and dt1 distributions, but the fast threshold seems to be determined incorrectly:
image

This is my XML config file (using strip map and ecal file from the unit tests):

<?xml version="1.0" encoding="UTF-8"?>
<StripEnergyThresholdFinder>
  <Input>
    <DataFiles>
      <DataFile>/home/hagemann/Downloads/gse_20260217T115220.hdf5</DataFile>
    </DataFiles>
    <CalibrationFile>/home/hagemann/Software/COSItools/nuclearizer/resource/unittestdata/406-1/hp52406-1.full.ecal</CalibrationFile>
    <StripMap>/home/hagemann/Software/COSItools/nuclearizer/resource/unittestdata/406-1/hp52406-1.stripmap.map</StripMap>
  </Input>
  <Output>
    <Prefix>output_file_prefix</Prefix>
  </Output>
  <Analysis>
    <MinEntries>10</MinEntries>
    <FallbackThresholdKeV>20</FallbackThresholdKeV>
    <NoiseSearchMaxKeV>40</NoiseSearchMaxKeV>
    <FastFallbackKeV>34</FastFallbackKeV>
  </Analysis>
</StripEnergyThresholdFinder>

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment on lines +953 to +957
thresholds[R] = m_FallbackThreshold;

// Mark the ADC value as invalid since we cannot easily convert back from
// the fallback keV value to ADC bins
thresholdsADC[R] = -1.0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation will give garbage values in thresholdsADC, but not in thresholds. Is this what we want?

continue;
}

hist->Smooth(3);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing?

Comment on lines +1036 to +1043
if (c > peakCounts) {

// Still climbing: update candidate peak
peakCounts = c;
peakBin = b;
declineCount = 0;

} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking that this is not an issue: some of the histogram bins seem empty in keV space, does this affect finding the leading edge?

Image

// the threshold a few bins into the flat top.
thresholdBin = pedestalThresholdBin;

} else if (R.GetStripID() == 64) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid hard-coded numbers in the code and put them somewhere at the top of the file as GR_STRIP_ID (this code might be used, and maybe the GR strip ID is not 64 then).

Comment on lines +1577 to +1591
// Require minimum statistics to avoid noise triggers
if (n0 + n1 < 50) {
continue;
}

if (n1 > n0) {
crossoverADC = ADC_val;
break;
}
}

int nbins = 21;

// Extend search window for stability
int searchMax = first_nonzero + 800;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of hard-coded values here, that we might define as constants towards the top of the file:
50 or minimum number of counts, 21 for nbins and 800 for extending the window.

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment on lines +1678 to +1694
MString HVOutputCSVFileName = m_OutputPrefix + "_Slow_HV_thresholds.csv";
MString LVOutputCSVFileName = m_OutputPrefix + "_Slow_LV_thresholds.csv";

ofstream csv_HV(HVOutputCSVFileName);
ofstream csv_LV(LVOutputCSVFileName);

if (csv_HV.is_open() == false) {
cerr << "Error: Failed to open CSV output file for HV thresholds: "
<< HVOutputCSVFileName << endl;
return;
}

if (csv_LV.is_open() == false) {
cerr << "Error: Failed to open CSV output file for LV thresholds: "
<< LVOutputCSVFileName << endl;
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to write everything to the same file instead of two separate ones.
Looks like the first token of every line already defines the side, so we should be able to write everything into one combined slow thresholds file (I think this is also what the forward pipeline would expect).

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment on lines +1730 to +1749
MString HardwareHVOutputCSVFileName =
m_OutputPrefix + "_Slow_Hardware_HV_thresholds.csv";

MString HardwareLVOutputCSVFileName =
m_OutputPrefix + "_Slow_Hardware_LV_thresholds.csv";

ofstream csv_Hardware_HV(HardwareHVOutputCSVFileName);
ofstream csv_Hardware_LV(HardwareLVOutputCSVFileName);

if (csv_Hardware_HV.is_open() == false) {
cerr << "Error: Failed to open CSV output file for HV hardware thresholds: "
<< HardwareHVOutputCSVFileName << endl;
return;
}

if (csv_Hardware_LV.is_open() == false) {
cerr << "Error: Failed to open CSV output file for LV hardware thresholds: "
<< HardwareLVOutputCSVFileName << endl;
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here: combine the two hardware thresholds file into one.

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated
Comment on lines +1791 to +1811
ofstream csv_TAC_HV(m_OutputPrefix + "_Fast_HV_thresholds.csv");
ofstream csv_TAC_LV(m_OutputPrefix + "_Fast_LV_thresholds.csv");

csv_TAC_HV << "detector_side,Strip,threshold_ADC,threshold_keV\n";
csv_TAC_LV << "detector_side,Strip,threshold_ADC,threshold_keV\n";

cout << "Writing FAST CSV entries: " << m_FastThresholds.size() << endl;

for (const auto& kv : m_FastThresholds) {
MReadOutElementDoubleStrip R = kv.first;
int Strip = R.GetStripID();

double thr_ADC = m_FastThresholdsADC[R];
double thr_keV = kv.second;

if (R.IsLowVoltageStrip() == true) {
csv_TAC_LV << "l," << Strip << "," << thr_ADC << "," << thr_keV << "\n";
} else {
csv_TAC_HV << "h," << Strip << "," << thr_ADC << "," << thr_keV << "\n";
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here: combine the two fast threshold files into one.

Comment thread apps/StripEnergyThresholdFinder.cxx Outdated

@fhagemann fhagemann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the app compiles and works when using multiple files.
You might want to remove the index from the XML file (it also works without it).

Comment on lines +5 to +6
<DataFile index="0">/path/to/your/data/file0.hdf5</DataFile>
<DataFile index="1">/path/to/your/data/file1.hdf5</DataFile>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<DataFile index="0">/path/to/your/data/file0.hdf5</DataFile>
<DataFile index="1">/path/to/your/data/file1.hdf5</DataFile>
<DataFile>/path/to/your/data/file0.hdf5</DataFile>
<DataFile>/path/to/your/data/file1.hdf5</DataFile>

… Hardware). Added hardware diagnostics in ADC units
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants