Skip to content

S30XL testbeam development #1676

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.

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

S30XL testbeam development #1676

wants to merge 3 commits into from

Conversation

bryngemark
Copy link
Contributor

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

Some changes to the setup compared to CERN testbeam 2022 (notably: number of time samples, number of bits actually used in TDC format, and the number of channels) meant that some tweaks to a series of producers and analyzers were due.

Check List

  • I successfully compiled ldmx-sw with my developments.
  • I read, understood and follow the coding rules.
  • I ran my developments and the following shows that they are successful.

some sample event of not-very-great-data from S30XL
sample event from SLAC testbench
example PE plot

@bryngemark bryngemark requested review from rodwyer100 and EBerzin April 3, 2025 08:44
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

I made a few suggestions, but did not put it to every occurance, for example for the verbose changes to trace

// for (int i = pedLength; i < 3*pedLength ; i++) {
////use 2nd and third 5th
if (charge.size() > 8) {
if (verbose_) ldmx_log(debug) << "going into oscillations check ";
Copy link
Member

Choose a reason for hiding this comment

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

here, I'd recommend to remove the verbose_ and move to ldmx_log(trace)

////use 2nd and third 5th
if (charge.size() > 8) {
if (verbose_) ldmx_log(debug) << "going into oscillations check ";
for (int i = 3; i < charge.size() - 4; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment on why we are skipping the first 2? is this supposed to be pedOffset+1? If yes, I'd prefer to move to a named constant than just a the hardcoded number

if (verbose_) ldmx_log(debug) << "going into oscillations check ";
for (int i = 3; i < charge.size() - 4; i++) {
float maxSamp = minCharge;
for (int iQ = 0; iQ < 4; iQ++) { // find the local max in the 4 samples
Copy link
Member

Choose a reason for hiding this comment

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

I guess a same comment about having the 4 a named constant

// ldmx_log(debug) << "got charge " << charge[i+iQ];
if (charge[i + iQ] > maxSamp) maxSamp = charge[i + iQ];
}
if (verbose_) ldmx_log(debug) << "got max charge " << maxSamp;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (verbose_) ldmx_log(debug) << "got max charge " << maxSamp;
ldmx_log(trace) << "got max charge " << maxSamp;

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.

2 participants