Skip to content

CRT Offline - Calib - #502

Merged
bear-is-asleep merged 1 commit into
feature/hlay_crt_offline_prfrom
feature/hlay_crt_offline_calib
Sep 26, 2024
Merged

CRT Offline - Calib#502
bear-is-asleep merged 1 commit into
feature/hlay_crt_offline_prfrom
feature/hlay_crt_offline_calib

Conversation

@henrylay97

@henrylay97 henrylay97 commented Aug 28, 2024

Copy link
Copy Markdown
Member

Description

This is part of a set of PRs designed to integrate updates to the CRT reconstruction to run it on data. To split up the diffs into more easily reviewable chunks there are 5 PRs to a common base branch. Once these have all been merged I will make a single PR to develop containing the full set of updates. Note, only on the final PR should the CI be run!

This particular PR introduces a simple service module that reads two text files containing pedestals and timing delays for each of the CRT channels / modules. This is a temporary solution which allows us to run a sensible first round of reconstruction. This will be replaced by use of the calibration database in the autumn.

Checklist

  • Added at least 1 label from available labels.
  • Assigned at least 1 reviewer under Reviewers,
  • Assigned all contributers including yourself under Assignees
  • Linked any relevant issues under Developement
  • Does this PR affect CAF data format? If so, please assign a CAF maintainer as additional reviewer.
  • Does this affect the standard workflow?

Relevant PR links (optional)

The final PR will rely on one in sbncode and one in sbnobj

Link(s) to docdb describing changes (optional)

CRT Data Reconstruction is detailed here: https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=36812

@henrylay97 henrylay97 added the crt Cosmic Ray Tagger label Aug 28, 2024
@henrylay97
henrylay97 requested a review from linyan-w August 28, 2024 14:20
@henrylay97 henrylay97 self-assigned this Aug 28, 2024

@absolution1 absolution1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good Henry.
I've added an optional comment about where to actually store the data. It shouldn't hold up the PR process though.

Comment thread sbndcode/Calibration/CRT/CalibService/CRTCalibService_service.cc

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a strong feeling, but these might be better placed in sbnd_data or stashcache? (we tend to store training weights in those places rather than github)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm happy to do so if we feel that would be better.

At the moment this follows the approach of the channel map directory (for relatively small text files) and longer term we will deprecate this method to make use of the calibration database.

@bear-is-asleep what is your take on this? Would you prefer they be moved to sbnd_data?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @henrylay97 , I think in general something like pedestals and detector responses should be stored in a database and not sbnd_data. It would be worth running this up the flag poll to a general analysis infrastructure discussion, since I don't think we've arrived at a consensus for where channel mappings, calibrations, etc. should go.

For now though, I think it'd be fine storing it where it is now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, I think we agree then. We absolutely want to move to the database as soon as is feasibly possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @bear-is-asleep and @henrylay97
The suggestion for using sbnd_data or stashcache is purely as temporary storage, until there's database access readily available.
I don't think we generally store large static files in git or other version control software.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as above about location

@bear-is-asleep

Copy link
Copy Markdown
Contributor

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_91_02

@FNALbuild

Copy link
Copy Markdown
Collaborator

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

@FNALbuild

Copy link
Copy Markdown
Collaborator

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

@FNALbuild

Copy link
Copy Markdown
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild

Copy link
Copy Markdown
Collaborator

❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for c14:prof - ignored failure for unit_test - ignored warnings for build -- details available through the CI dashboard

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

parent CI build details are available through the CI dashboard

@bear-is-asleep
bear-is-asleep merged commit a46a522 into feature/hlay_crt_offline_pr Sep 26, 2024
@henrylay97 henrylay97 mentioned this pull request Sep 26, 2024
6 tasks
@henrylay97
henrylay97 deleted the feature/hlay_crt_offline_calib branch November 21, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crt Cosmic Ray Tagger

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants