Digitization algorithm that reproduces the HGCROC measurement #2169

mhkim-anl · 2025-10-30T21:30:58Z

Briefly, what does this PR introduce?

This PR is to reproduce the HGCROC measurement and store the corresponding data types, RawHGCROCHit and HGCROCSample, from pulse. When HGCROC measures the TOT, it is stored in the same sample where the TOA is measured even though it is measured during a sample that is different from the TOA one. This means we should not finalize/output the sample immediately after scanning a time window in the code. Rather, we need to temporarily store the samples somewhere (like a buffer) until the TOT is measured or all the amplitudes are scanned. For this purpose, a class, HGCROCRawSample was used in this algorithm.

Please refer to the slides on the basic structure of this digitization algorithm.

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue Digitization algorithm to measure ADC, TOA, and TOT #2090)
Documentation update
Other: __

Please check if this PR fulfills the following:

Tests for the changes have been added
Documentation has been added / updated
Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

No

veprbl

I have some early comments.

veprbl · 2025-11-04T19:13:01Z

src/algorithms/digi/PulseDigi.h

+
+#include <algorithms/algorithm.h>
+#include <edm4eic/SimPulseCollection.h>
+#include <edm4eic/RawHGCROCHitCollection.h>


You will need to require EDM4eic 8.3.0 to use your code. Basically, see some of older changes removed in https://github.com/eic/EICrecon/pull/2121/files for inspiration.

Thank you for pointing out the EDM4eic version requirement. I believe I understood the changes in the page you linked, and have updated the EDM4EIC_VERSION_MIN 8.0 to 8.3.0. Please let me know if I've misunderstood your comments.

veprbl · 2025-11-04T19:13:31Z

src/factories/digi/PulseDigi_factory.h

+
+namespace eicrecon {
+
+class PulseDigi_factory : public JOmniFactory<PulseDigi_factory, PulseDigiConfig> {


Please come up with a more descriptive name than PulseDigi.

veprbl · 2025-11-04T19:14:49Z

src/algorithms/digi/PulseDigi.cc

This algorithm needs a use somewhere. We don't add dead code. Either make a unit test or include this in the analysis chain.

veprbl · 2025-11-04T19:17:19Z

src/factories/digi/PulseDigi_factory.h

+  ParameterRef<double> m_time_window{this, "time_window", config().time_window};
+  ParameterRef<double> m_adc_phase{this, "adc_phase", config().adc_phase};
+  ParameterRef<double> m_toa_thres{this, "toa_thres", config().toa_thres};
+  ParameterRef<double> m_tot_thres{this, "tot_thres", config().tot_thres};
+  ParameterRef<unsigned int> m_capADC{this, "capADC", config().capADC};
+  ParameterRef<double> m_dyRangeADC{this, "dyRangeADC", config().dyRangeADC};
+  ParameterRef<unsigned int> m_capTOA{this, "capTOA", config().capTOA};
+  ParameterRef<double> m_dyRangeTOA{this, "dyRangeTOA", config().dyRangeTOA};
+  ParameterRef<unsigned int> m_capTOT{this, "capTOT", config().capTOT};
+  ParameterRef<double> m_dyRangeTOT{this, "dyRangeTOT", config().dyRangeTOT};


Suggested change

ParameterRef<double> m_time_window{this, "time_window", config().time_window};

ParameterRef<double> m_adc_phase{this, "adc_phase", config().adc_phase};

ParameterRef<double> m_toa_thres{this, "toa_thres", config().toa_thres};

ParameterRef<double> m_tot_thres{this, "tot_thres", config().tot_thres};

ParameterRef<unsigned int> m_capADC{this, "capADC", config().capADC};

ParameterRef<double> m_dyRangeADC{this, "dyRangeADC", config().dyRangeADC};

ParameterRef<unsigned int> m_capTOA{this, "capTOA", config().capTOA};

ParameterRef<double> m_dyRangeTOA{this, "dyRangeTOA", config().dyRangeTOA};

ParameterRef<unsigned int> m_capTOT{this, "capTOT", config().capTOT};

ParameterRef<double> m_dyRangeTOT{this, "dyRangeTOT", config().dyRangeTOT};

ParameterRef<double> m_time_window{this, "timeWindow", config().time_window};

ParameterRef<double> m_adc_phase{this, "adcPhase", config().adc_phase};

ParameterRef<double> m_toa_thres{this, "toaThres", config().toa_thres};

ParameterRef<double> m_tot_thres{this, "totThres", config().tot_thres};

ParameterRef<unsigned int> m_capADC{this, "capADC", config().capADC};

ParameterRef<double> m_dyRangeADC{this, "dyRangeADC", config().dyRangeADC};

ParameterRef<unsigned int> m_capTOA{this, "capTOA", config().capTOA};

ParameterRef<double> m_dyRangeTOA{this, "dyRangeTOA", config().dyRangeTOA};

ParameterRef<unsigned int> m_capTOT{this, "capTOT", config().capTOT};

ParameterRef<double> m_dyRangeTOT{this, "dyRangeTOT", config().dyRangeTOT};

camelCase for JANA2 parameter names, and underscores for config names. Bonus points for making variable names consistent with what is used elsewhere.

Thank you for pointing out that the JANA2 parameters were not using camelCase. I've applied your suggested changes in PulseDigi_factory.h.
For the parameter names in PulseDigiConfig.h, would it be better to use underscores consistently for all of them? Since CalorimeterHitDigiConfig uses camelCase for its parameters (it has capADC and dyRangeADC), it is a bit hard for me to decided, so I wanted to follow your suggestion.

wdconinc · 2025-11-16T23:53:57Z

@mhkim-anl Can you please first address @veprbl's early comments, and introduce sufficient wiring that this actually gets used? It should also pass compilation checks (use version or has_include guards around output datatype sections, but the calculations need to be enabled). Then I can take a look and review.

src/algorithms/digi/PulseDigi.h

mhkim-anl · 2025-11-17T20:04:21Z

@veprbl @wdconinc
I'm very sorry if my action caused any misunderstanding. After @veprbl left a couple of comments, I've been studying the dynamic ranges of the ADC, TOA, and TOT to use this algorithm for BIC regarding the following comment.

This algorithm needs a use somewhere. We don't add dead code. Either make a unit test or include this in the analysis chain.

In the meantime, I added reviewers because this PR was presented at the last simulation meeting, Slides. I was going to add reviewers after presenting the first version of the digitization algorithm.

I'll address and resolve @veprbl's early comments first for sure. Since my studies on BIC's ADC, TOA, and TOT dynamic ranges are completing, I'll update the codes based on @veprbl and @wdconinc's comments soon. Thanks!

Co-authored-by: Dmitry Kalinkin <[email protected]>

mhkim-anl · 2025-11-23T20:58:40Z

Regarding @veprbl's and @wdconinc's following comments,

Please come up with a more descriptive name than PulseDigi

This algorithm needs a use somewhere.

introduce sufficient wiring that this actually gets used

Since BIC has come to prefer CALOROC1B chip over HGCROC, I've implemented CALOROC1B data model (PR) and I'm now considering whether this algorithm should reproduce the HGCROC measurement, the CALOROC1B measurement, or both.

I understood that only the HGCROC data model has been implemented from @ruse-traveler's previous PR because the CALOROC1B chip is not available yet. However, if we implement the CALOROC1B data model, we can compare its performance with the HGCROC-based one and use this to help decide which chip will ultimately be used for the ePIC calorimeters. This study will also provide useful input for the chip development, including parameter optimization such as thresholds and dynamic ranges.

Therefore, I would like to discuss the CALOROC1B data model implementation and the direction of this algorithm at the upcoming BIC simulation meeting. I'll invite reviewers to that discussion.

mhkim-anl added 13 commits September 22, 2025 17:34

First commit for PulseDigi

df0b9af

code-formatter applied

43b2761

Fix typo

11e9eea

More optimized to run efficiently

6202d54

clang-format applied

89199f6

Some typos were fixed

5e57416

Add some explanations

766dc84

Combined two member functions

5424434

Added const

4eb18fd

Amplitudes are measured using tick

5d336d4

add -> set

4d621f2

Update for PR

ecf9fd7

code formatter has been applied

594f668

mhkim-anl linked an issue Oct 30, 2025 that may be closed by this pull request

Digitization algorithm to measure ADC, TOA, and TOT #2090

Open

github-actions bot added the topic: digitization label Oct 30, 2025

Merge branch 'main' into 2090-digitization-algorithm

ab5625c

mhkim-anl changed the title ~~Fill HGCROC sample from pulse~~ Digitization algorithm that reproduces the HGCROC measurement Oct 31, 2025

mhkim-anl self-assigned this Oct 31, 2025

veprbl reviewed Nov 4, 2025

View reviewed changes

mhkim-anl requested review from mariakzurek, ruse-traveler, sly2j and wdconinc November 16, 2025 23:43

wdconinc reviewed Nov 16, 2025

View reviewed changes

src/algorithms/digi/PulseDigi.h Outdated Show resolved Hide resolved

mhkim-anl marked this pull request as draft November 22, 2025 23:33

mhkim-anl and others added 3 commits November 22, 2025 17:34

Merge branch 'main' into 2090-digitization-algorithm

b078503

Update src/factories/digi/PulseDigi_factory.h

939bb51

Co-authored-by: Dmitry Kalinkin <[email protected]>

Removed an unused variable

62efe5d

Considered EDM4EIC_VERSION_MIN

dddddf8

github-actions bot added the topic: infrastructure label Nov 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Digitization algorithm that reproduces the HGCROC measurement #2169

Digitization algorithm that reproduces the HGCROC measurement #2169

mhkim-anl commented Oct 30, 2025 •

edited

Loading

Uh oh!

veprbl left a comment

Uh oh!

veprbl Nov 4, 2025

Uh oh!

mhkim-anl Nov 23, 2025

Uh oh!

veprbl Nov 4, 2025

Uh oh!

veprbl Nov 4, 2025

Uh oh!

veprbl Nov 4, 2025

Uh oh!

mhkim-anl Nov 23, 2025

Uh oh!

wdconinc commented Nov 16, 2025

Uh oh!

Uh oh!

mhkim-anl commented Nov 17, 2025

Uh oh!

mhkim-anl commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		namespace eicrecon {

		class PulseDigi_factory : public JOmniFactory<PulseDigi_factory, PulseDigiConfig> {

Digitization algorithm that reproduces the HGCROC measurement #2169

Are you sure you want to change the base?

Digitization algorithm that reproduces the HGCROC measurement #2169

Conversation

mhkim-anl commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

Uh oh!

veprbl left a comment

Choose a reason for hiding this comment

Uh oh!

veprbl Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

mhkim-anl Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

veprbl Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

veprbl Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

veprbl Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

mhkim-anl Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

wdconinc commented Nov 16, 2025

Uh oh!

Uh oh!

mhkim-anl commented Nov 17, 2025

Uh oh!

mhkim-anl commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mhkim-anl commented Oct 30, 2025 •

edited

Loading