Skip to content

Add ADC pulses to Store #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marc1uk merged 8 commits into from
Oct 22, 2025
Merged

Add ADC pulses to Store #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2f97474
Update README.md
S81D Sep 23, 2025
812801d
Update PhaseIIADCHitFinder.cpp
S81D Sep 23, 2025
2eabe6c
Update ADCPulse.cpp
S81D Sep 23, 2025
d0dd88a
Update ADCPulse.h
S81D Sep 23, 2025
50d88fb
Update ADCPulse.h
S81D Sep 23, 2025
3795074
Update ADCPulse.cpp
S81D Sep 23, 2025
e14e79c
Update PhaseIIADCHitFinder.cpp
S81D Sep 23, 2025
a64f0e4
Update ADCPulse.h
S81D Sep 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions DataModel/ADCPulse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
ADCPulse::ADCPulse(int TubeId, double start_time, double peak_time,
double baseline, double sigma_baseline, unsigned long area,
unsigned short raw_amplitude, double calibrated_amplitude,
double charge) : Hit(TubeId, start_time, charge),
double charge, const std::vector<double>& trace_x, const std::vector<double>& trace_y) :
Hit(TubeId, start_time, charge),
start_time_(start_time), peak_time_(peak_time),
baseline_(baseline), sigma_baseline_(sigma_baseline), raw_area_(area),
raw_amplitude_(raw_amplitude), calibrated_amplitude_(calibrated_amplitude)
raw_amplitude_(raw_amplitude), calibrated_amplitude_(calibrated_amplitude),
trace_x_(trace_x), trace_y_(trace_y)
{
}
21 changes: 19 additions & 2 deletions DataModel/ADCPulse.h
View file
Open in desktop
Copy link
Collaborator

@jminock jminock Sep 24, 2025

Choose a reason for hiding this comment

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

No functionality is being taken away, and the changes would only impact the ADCHitFinder to my knowledge; so, I don't think there's anything wrong with making changes to the DataModel in principle. Will need further review from Ben and/or Marcus regarding the version conditional.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// ToolAnalysis includes
#include "ChannelKey.h"
#include "Hit.h"
#include <vector>

class ADCPulse : public Hit {

Expand All @@ -22,8 +23,9 @@ class ADCPulse : public Hit {
// int TubeId member
ADCPulse(int TubeId, double start_time, double peak_time,
double baseline, double sigma_baseline, unsigned long raw_area,
unsigned short raw_amplitude, double calibrated_amplitude,
double charge);
unsigned short raw_amplitude, double calibrated_amplitude, double charge,
const std::vector<double>& trace_x = std::vector<double>(),
Copy link
Collaborator

@marc1uk marc1uk Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's very odd for the constructor to be taking references as arguments, and in general references should not be initialised to temporaries (I'm a little surprised the compiler let you do it). I suppose ultimately it may work, but I would make this non-references to avoid confusion.

Copy link
Collaborator Author

@S81D S81D Oct 10, 2025

Choose a reason for hiding this comment

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

Yeah I agree it is odd / not standard. I haven't noticed any issues though and it seems to behave fine with the event building. We can flag it moving forward if you want.

Copy link
Collaborator

@marc1uk marc1uk Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If it's odd/non-standard, then it's probably something to not do. 😅
Assuming the point here is to avoid a second copy as part of constructing this object from optional existing data, i think the correct way to do it would be to accept a pointer that defaults to null, and to make the single copy within the constructor if the passed pointer is not null.

const std::vector<double>& trace_y = std::vector<double>());

// @brief Returns the start time (ns) of the pulse relative to the
// start of its minibuffer
Expand Down Expand Up @@ -59,6 +61,10 @@ class ADCPulse : public Hit {
// (baseline-subtracted) pulse
inline double amplitude() const { return calibrated_amplitude_; }

// @brief Returns the x [ns] and y [ADC] points of the "found" pulse (baseline-subtracted and relative to pulse start point)
inline const std::vector<double>& GetTraceXPoints() const { return trace_x_; }
Copy link
Collaborator

@marc1uk marc1uk Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

functions should not return references to member variables.
If the ADCPulse object is destroyed (goes out of scope or gets deleted), a caller may be holding onto a dangling reference to a member of an object that no longer exists (i.e., a segfault waiting to happen). Just return by value and if the caller is smart, they can capture that into a reference.

Copy link
Collaborator Author

@S81D S81D Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for pointing this out. I agree that returning by value is safer. I have not observed any problems in my workflow but its likely because any instances I'm fetching it, its lifetime is short.

In practice I was having some troubles getting something to work, and this ended up working without slowing down the event building or giving any observed issues (both creating the ProcessedData and reading from it). One potential problem I have with changing it is the potential performance impact. This object is created and accessed millions of times per run (15k triggers per part file, 128 PMTs, many hits per event). A real concern is the potential slowdown if returning by value and copying the vector millions of times when churning through the (already) slow event building. I unfortunately don't have a ton of time to stress test this (but have stress tested this change ~thousands of times.

If I had to guess these traces will not be used often as analyzers are largely focused on the hits information. It was mostly added just in case anyone wants it down the line. I think for potential performance concerns (and limited time) we could keep it unless you feel strongly otherwise.

inline const std::vector<double>& GetTraceYPoints() const { return trace_y_; }

template <class Archive> void serialize(Archive& ar,
const unsigned int version)
{
Expand All @@ -71,6 +77,10 @@ class ADCPulse : public Hit {
ar & raw_area_;
ar & raw_amplitude_;
ar & calibrated_amplitude_;
if (version > 0) {
Copy link
Collaborator

@jminock jminock Sep 24, 2025

Choose a reason for hiding this comment

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

How does version work? How does it iterate? Is this the correct way to ensure the version differences? These are questions more for Ben and Marcus.

Copy link
Collaborator

@marc1uk marc1uk Oct 9, 2025

Choose a reason for hiding this comment

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

pretty much the same as ROOT's schema evolution - the default class version is 0. If you modify the class definition, you need to manually alter the class version (what value you give it is up to you) and the (de)serialisation code to check that and behave appropriately. I don't believe there's any automatic way to ensure it does the 'correct' thing.

ar & trace_x_;
ar & trace_y_;
}
}

protected:
Expand All @@ -83,4 +93,11 @@ class ADCPulse : public Hit {

unsigned short raw_amplitude_; // ADC
double calibrated_amplitude_; // V

std::vector<double> trace_x_ = {}; // x points of the pulse (start at 0, relative to pulse start) [ns]
std::vector<double> trace_y_ = {}; // y points of the pulse (baseline-subtracted) [ADC]
};

// (From Andrew Sutton) Need to increment the class version since we added time as a new variable
// the version number ensures backward compatibility when serializing
BOOST_CLASS_VERSION(ADCPulse, 1)
81 changes: 70 additions & 11 deletions UserTools/PhaseIIADCHitFinder/PhaseIIADCHitFinder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,18 @@ bool PhaseIIADCHitFinder::Initialise(std::string config_filename, DataModel& dat
m_data = &data;

// Load the default threshold settings for finding pulses
verbosity = 3;
verbosity = 0;
use_led_waveforms = false;
pulse_finding_approach = "threshold";
adc_threshold_db = "none";
default_adc_threshold = 5;
default_adc_threshold = 7;
threshold_type = "relative";
pulse_window_type = "fixed";
pulse_window_type = "Fixed_2023_Gains";
pulse_window_start_shift = -3;
pulse_window_end_shift = 25;
adc_window_db = "none"; //Used when pulse_finding_approach="fixed_windows"
eventbuilding_mode = false;
mc_waveforms = false;

//Load any configurables set in the config file
m_variables.Get("verbosity",verbosity);
Expand Down Expand Up @@ -781,13 +782,28 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bywindow(
}
}

// extract the x and y points of the pulse (subtract off baseline and "zero" the pulse to the pulse start)
Copy link
Collaborator

@marc1uk marc1uk Oct 9, 2025

Choose a reason for hiding this comment

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

this same block is added in four times in this PR. Can we make it a function?

Copy link
Collaborator Author

@S81D S81D Oct 10, 2025

Choose a reason for hiding this comment

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

There's a lot of duplicated code in this tool (different pulse-finding approaches are very similar; it's not the most elegant tool). It agree it would be worth while to re-organize this tool into more concise functions. Could we for now add it as a TODO? Rather than making this particular feature its own function, since much of the pulse-finding approaches use the same infrastructure it could instead be folded into a single function that could be called (more than just the trace extraction).

Copy link
Collaborator

@marc1uk marc1uk Oct 21, 2025

Choose a reason for hiding this comment

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

i'm inclined to think that the fact that this code is added as part of a new feature, even if this was to be refactored, it would be fair for it to be a separate function. Besides, even if the code could do with refactoring, putting unrelated improvements off as TODO is one thing, but making the situation worse is something esle. It shouldn't be a difficult change.

std::vector<double> trace_x;
std::vector<double> trace_y;

double pulse_start_time = wmin * NS_PER_ADC_SAMPLE;
double pulse_baseline = calibrated_minibuffer_data.GetBaseline();

for (size_t p = wmin; p <= wmax; ++p) {
double ns_time = p * NS_PER_ADC_SAMPLE;
double val_adc = raw_minibuffer_data.GetSample(p);
trace_x.push_back(ns_time - pulse_start_time);
trace_y.push_back(val_adc - pulse_baseline);
}

// Store the freshly made pulse in the vector of found pulses
pulses.emplace_back(channel_key,
( wmin * NS_PER_SAMPLE )-timing_offset,
Copy link
Collaborator

@marc1uk marc1uk Oct 9, 2025

Choose a reason for hiding this comment

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

i'm curious what this change is about? I can't spot a variable rename from NS_PER_SAMPLE to NS_PER_ADC_SAMPLE....

Copy link
Collaborator Author

@S81D S81D Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The tool calls both (and both are defined the same way in constants):

ToolAnalysis/DataModel/Constants.h

Line 5 in 9187a75

constexpr unsigned int NS_PER_SAMPLE = 2; // ns
and

ToolAnalysis/DataModel/ANNIEconstants.h

Line 17 in 9187a75

constexpr unsigned int NS_PER_ADC_SAMPLE = 2; // ns

I was trying to make it all consistent. It functionally doesn't change anything.

Copy link
Collaborator

@marc1uk marc1uk Oct 21, 2025

Choose a reason for hiding this comment

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

i think this pushes the question upstream - why are there two constants that appear to both represent the same thing. "The tool calls both" is not a good thing (and doesn't really explain why this instance in particular should be changed).
Can we do a find-and-replace and remove the redundancy?

(peak_sample * NS_PER_SAMPLE)-timing_offset,
( wmin * NS_PER_ADC_SAMPLE )-timing_offset,
(peak_sample * NS_PER_ADC_SAMPLE)-timing_offset,
calibrated_minibuffer_data.GetBaseline(),
calibrated_minibuffer_data.GetSigmaBaseline(),
raw_area, max_ADC, calibrated_amplitude, charge);
raw_area, max_ADC, calibrated_amplitude, charge,
trace_x, trace_y);
}
return pulses;
}
Expand Down Expand Up @@ -895,13 +911,28 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bythreshold(
}
}

// extract the x and y points of the pulse (subtract off baseline and "zero" the pulse to the pulse start)
std::vector<double> trace_x;
std::vector<double> trace_y;

double pulse_start_time = pulse_start_sample * NS_PER_ADC_SAMPLE;
double pulse_baseline = calibrated_minibuffer_data.GetBaseline();

for (size_t p = pulse_start_sample; p <= pulse_end_sample; ++p) {
double ns_time = p * NS_PER_ADC_SAMPLE;
double val_adc = raw_minibuffer_data.GetSample(p);
trace_x.push_back(ns_time - pulse_start_time);
trace_y.push_back(val_adc - pulse_baseline);
}

// Store the freshly made pulse in the vector of found pulses
pulses.emplace_back(channel_key,
( pulse_start_sample * NS_PER_SAMPLE )-timing_offset,
(peak_sample * NS_PER_SAMPLE)-timing_offset,
( pulse_start_sample * NS_PER_ADC_SAMPLE )-timing_offset,
(peak_sample * NS_PER_ADC_SAMPLE)-timing_offset,
calibrated_minibuffer_data.GetBaseline(),
calibrated_minibuffer_data.GetSigmaBaseline(),
raw_area, max_ADC, calibrated_amplitude, charge);
raw_area, max_ADC, calibrated_amplitude, charge,
trace_x, trace_y);
}


Expand Down Expand Up @@ -1052,6 +1083,19 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bythreshold(
}
}

// extract the x and y points of the pulse (subtract off baseline and "zero" the pulse to the pulse start)
std::vector<double> trace_x;
std::vector<double> trace_y;
double pulse_start_time = pulse_start_sample * NS_PER_ADC_SAMPLE;
double pulse_baseline = calibrated_minibuffer_data.GetBaseline();

for (size_t p = pulse_start_sample; p <= pulse_end_sample; ++p) {
double ns_time = p * NS_PER_ADC_SAMPLE;
double val_adc = raw_minibuffer_data.GetSample(p);
trace_x.push_back(ns_time - pulse_start_time);
trace_y.push_back(val_adc - pulse_baseline);
}

if(verbosity>v_debug) std::cout << "PhaseIIADCHitFinder: Hit time [ns] " << hit_time * NS_PER_ADC_SAMPLE << std::endl;

if (hit_time < 0.0) {
Expand All @@ -1074,7 +1118,8 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bythreshold(
( hit_time * NS_PER_ADC_SAMPLE )-timing_offset, // interpolated hit time
calibrated_minibuffer_data.GetBaseline(),
calibrated_minibuffer_data.GetSigmaBaseline(),
raw_area, max_ADC, calibrated_amplitude, charge);
raw_area, max_ADC, calibrated_amplitude, charge,
trace_x, trace_y);
}
}

Expand Down Expand Up @@ -1174,6 +1219,19 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bythreshold(
}
}

// extract the x and y points of the pulse (subtract off baseline and "zero" the pulse to the pulse start)
std::vector<double> trace_x;
std::vector<double> trace_y;
double pulse_start_time = pulse_start_sample * NS_PER_ADC_SAMPLE;
double pulse_baseline = calibrated_minibuffer_data.GetBaseline();

for (size_t p = pulse_start_sample; p <= pulse_end_sample; ++p) {
double ns_time = p * NS_PER_ADC_SAMPLE;
double val_adc = raw_minibuffer_data.GetSample(p);
trace_x.push_back(ns_time - pulse_start_time);
trace_y.push_back(val_adc - pulse_baseline);
}

if (hit_time < 0.0) {
// If for some reason the interpolation finds a negative time value (if the pulse is extremely early in the buffer),
// default to the peak time (maximum ADC value of the pulse)
Expand Down Expand Up @@ -1202,7 +1260,8 @@ std::vector<ADCPulse> PhaseIIADCHitFinder::find_pulses_bythreshold(
(hit_time * NS_PER_ADC_SAMPLE)-timing_offset, // interpolated hit time
calibrated_minibuffer_data.GetBaseline(),
calibrated_minibuffer_data.GetSigmaBaseline(),
raw_area, max_ADC, calibrated_amplitude, charge);
raw_area, max_ADC, calibrated_amplitude, charge,
trace_x, trace_y);
}
}
} else {
Expand Down
Loading