Skip to content

Fix bugs and code quality issues found in CRVResponse review#1789

Merged
oksuzian merged 2 commits intomainfrom
copilot/review-crvresponse-issues
Apr 8, 2026
Merged

Fix bugs and code quality issues found in CRVResponse review#1789
oksuzian merged 2 commits intomainfrom
copilot/review-crvresponse-issues

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026

Summary

Detailed code review of the CRVResponse module identified and fixed several bugs and code quality issues across 4 files.

Bug Fixes

1. Wrong logarithm base in SiPM after-pulse timing (MakeCrvSiPMCharges.cc)

The exponential distribution sampling for trap (after-pulse) times used log10() instead of log() (natural logarithm). The standard formula for exponential random variable generation is x = -lifetime * ln(U) where U is uniform(0,1). Using log10 produced a mean of lifetime / ln(10) ≈ lifetime / 2.303, making after-pulse times ~2.3× shorter than intended by the physical lifetime parameters.

2. Operator precedence bug in findScintillatorCerenkovBinReverse (MakeCrvPhotons.cc)

The expression (bin / nBetaBins*nZBins) % nYBins evaluated as ((bin / nBetaBins) * nZBins) % nYBins due to left-to-right associativity of / and *, instead of the intended (bin / (nBetaBins*nZBins)) % nYBins. This affected the reverse bin lookup used in diagnostics.

3. Cherenkov photon interpolation bug (MakeCrvPhotons.cc)

In GetAverageNumberOfCerenkovPhotons, the prevBeta/prevNumberPhotons values for linear interpolation were only updated on the first map iteration. For beta values beyond the second map entry, the function would incorrectly interpolate between the first entry and the matching entry, instead of between adjacent entries.

4. Wrong type for NZS flag (CrvDigitizer_module.cc)

crvDigiMC.IsNZS() returns bool but was stored in a double variable.

Code Quality Improvements

  1. Removed dead code: Removed unused static int nPScintillation/nPCerenkov counters and their associated commented-out print statement in MakeCrvPhotons.cc.
  2. Fixed misleading comments: Corrected swapped closing-brace comments in the nested SiPM/step/photon loops in MakeCrvPhotons::MakePhotons.
  3. Fixed typo: "dublicate" → "duplicate" in CrvWaveformsGenerator_module.cc.
  4. Improved error handling: Changed TFile open check from == NULL (which doesn't detect failed opens since new never returns NULL) to IsZombie() in MakeCrvSiPMCharges.cc.

Copilot AI and others added 2 commits April 8, 2026 01:17
- Fix log10 -> log for exponential distribution in SiPM after-pulse timing
- Fix operator precedence in findScintillatorCerenkovBinReverse
- Fix Cherenkov photon interpolation (update prev on every iteration)
- Fix double -> bool for NZS flag in CrvDigitizer
- Remove dead static counters in MakeCrvPhotons
- Fix swapped closing-brace comments in MakeCrvPhotons
- Fix typo 'dublicate' -> 'duplicate'
- Improve TFile error checking with IsZombie()

Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/3fe2c6a5-2029-4a5b-9bd1-a38d758173b9

Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com>
@FNALbuild
Copy link
Copy Markdown
Collaborator

Hi @Copilot,
You have proposed changes to files in these packages:

  • CRVResponse

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

📝 The author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

@oksuzian
Copy link
Copy Markdown
Collaborator

oksuzian commented Apr 8, 2026

@FNALbuild run build test

@oksuzian oksuzian marked this pull request as ready for review April 8, 2026 04:10
@FNALbuild
Copy link
Copy Markdown
Collaborator

⌛ The following tests have been triggered for 3dc2285: build (Build queue - API unavailable)

@ehrlich-uva
Copy link
Copy Markdown
Contributor

ehrlich-uva commented Apr 8, 2026

All proposed changes can be accepted. Only point 3 will result in an insignificant change of the simulation results. Here are some additional comments.

  1. This calculation is used for after-pulses, which were never turned on in our simulations.
  2. This reverse lookup function is not used in our simulations.
  3. This function is used for the simulation of Cerenkov photons, but the number of Cerenkov photons is small compared to the scintillator photons, and the difference in the number of Cerenkov photons introduced by this bug is also very small. So overall, this bug has only an insignificant impact on the number of photons.
  4. This bug did not make a difference, since the NZS flag in CrvDigi is currently not checked anywhere.
  5. It was used for debugging and can be removed.
  6. These comments must have been switched by mistake.
  7. Typo in comment.
  8. It never caused any problems, but doesn't hurt to change.

Copy link
Copy Markdown
Contributor

@ehrlich-uva ehrlich-uva left a comment

Choose a reason for hiding this comment

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

See my comments above.

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 3dc2285.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 3dc2285 at 5ec09f9
build (prof) Log file. Build time: 04 min 07 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 4 files
clang-tidy ➡️ 4 errors 100 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 3dc2285 after being merged into the base branch at 5ec09f9.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian oksuzian merged commit 3011611 into main Apr 8, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants