Skip to content

Move PMTBeamSignal data product from icaruscode to sbnobj #832

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 10 commits into
base: develop
Choose a base branch
from

Conversation

aheggest
Copy link
Contributor

@aheggest aheggest commented Jun 2, 2025

This pull request (together with sbnobj PR #130) moves the PMTBeamSignal data product from icaruscode/IcarusObj to a SBN common area, sbnobj/Common. This enables the special RWM signals digitized by the PMTs to be read into the CAFs.

The main changes in this PR are switching all icarus::timing::PMTBeamSignal --> sbn::timing::PMTBeamSignal and removing the previous defintion in PMTBeamSignal.h.

This PR is a part of a PR pair:

To fully get the information into the CAFs, sbncode PR#541 and sbnanaobj PR#141 should also be merged in, but this icaruscode PR #832 and sbnobj PR #130 do not rely on sbncode PR#541 and sbnanaobj PR#141.

Added a simple producer module PMTBeamSignalNameSpaceConverter_module.cc to convert the PMTBeamSignal from icarus::timing namespace to sbn::timing. This is to help with backward compatibility for files that were produced with icarus::timing::PMTBeamSignal - we can convert the data product to the correct sbn::timing namespace needed to be read into the CAFs.

Copy link
Member

@mvicenzi mvicenzi left a comment

Choose a reason for hiding this comment

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

This looks pretty straighforward, but I have just two minor comments below.

@aheggest aheggest self-assigned this Jun 2, 2025
@aheggest aheggest marked this pull request as draft June 3, 2025 17:41
@aheggest aheggest moved this from Open pull requests to PR in progress in SBN software development Jun 3, 2025
@aheggest aheggest marked this pull request as ready for review June 6, 2025 19:56
Copy link
Member

@mvicenzi mvicenzi left a comment

Choose a reason for hiding this comment

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

Few more comments on the recent additions.
The most important is making the namespace converter more general to be able to handle both RWM and EW products. There is no current need for it, but you never know.

@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jun 12, 2025
@leoaliaga
Copy link
Contributor

trigger build larsoft@v10_06_00 LArSoft/lar*@LARSOFT_SUITE_v10_06_00 SBNSoftware/sbnalg@v10_06_00_01 SBNSoftware/sbncode@v10_06_00_01 SBNSoftware/sbnobj#130

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: PR in progress
Development

Successfully merging this pull request may close these issues.

4 participants