-
Notifications
You must be signed in to change notification settings - Fork 54
addign time dependent etau correction and global tag for calib DB tag for TPC etau and PDS gain #873
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
addign time dependent etau correction and global tag for calib DB tag for TPC etau and PDS gain #873
Conversation
| if (cryo == 0 && tpc == 1) thiselifetime = runelifetime.tau_W; | ||
|
|
||
| // Get the hit time | ||
| double thit = hit.PeakTime()/2000. - 0.2 - t0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @henrylay97 , if you get a chance, could you please check if this hit time reconstruction part looks okay? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this should be replaced with values taken from fClockData. At the moment you set it up but don't use it, that will be much better than hard coding the values. You can take inspiration from the lifetime correction in the CalorimetryAlg. Units can sometime be a problem so it's worth sanity checking the outputs.
| service_provider: "PMTCalibrationDatabaseService" | ||
| CorrectionTags: { | ||
| PMTCalibrationDatabaseTag: "v1r1" | ||
| PMTCalibrationDatabaseTag: @local::SBND_Calibration_GlobalTags.PMTCalibrationDatabaseTag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @asanchezcastillo , if you get a chance, could you please check if this update is okay for you? thank you!
sbndcode/Calibration/configurations/calibration_database_PDS_TagSets_sbnd.fcl
Outdated
Show resolved
Hide resolved
| CorrectionTags: { | ||
| PMTCalibrationDatabaseTag: "v1r1" | ||
| PMTCalibrationDatabaseTag: @local::SBND_Calibration_GlobalTags.PMTCalibrationDatabaseTag | ||
| DatabaseTimeStamp: 1757601071000000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DatabaseTimeStamp: 1757601071000000000 | |
| DatabaseTimeStamp: @local::SBND_Calibration_GlobalTags.DatabaseTimeStamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the previous suggestions, the timestamp should be read from the configuration file.
|
Hi @sungbinoh ! Thanks for the extremely useful work. Overall the PR looks good to me. Once the suggested configuration changes are committed we can move on and merge. I will update #863 to include these changes as well. |
linyan-w
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve once the timestamp is updated!
Hi @asanchezcastillo , thank you the review and comments! As a sanity check, in the dumped |
henrylay97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sungbin! Thanks for this it looks like a significant chunk of work. I have a couple of suggestions on implementation. As discussed offline I also think it would be great to have some kind of validation of this, any simple mistake (e.g. typo) here will throw off all of our data calorimetry in the next production.
| BEGIN_PROLOG | ||
|
|
||
| SBND_Calibration_GlobalTags: { | ||
| @table::TPC_CalibrationTags_Nov2025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we also have a CRT calib table currently in an approved PR. Depending on what gets merged first we should make sure the CRT implementation get's swept into this more generic setup :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now merged, would you have time today to look at how to ensure consistency with this? If not I can look in the morning my time!
| double sbnd::calo::NormalizeDriftSQLite::Normalize(double dQdx, const art::Event &e, | ||
| const recob::Hit &hit, const geo::Point_t &location, const geo::Vector_t &direction, double t0) { | ||
|
|
||
| assert(fClockData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe assert only works in debug builds? Do we want to change this so an exception would be thrown in production mode if the clock data was invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the assert line to an if condition to throw exception when fClockData is not valid.
| if (cryo == 0 && tpc == 1) thiselifetime = runelifetime.tau_W; | ||
|
|
||
| // Get the hit time | ||
| double thit = hit.PeakTime()/2000. - 0.2 - t0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this should be replaced with values taken from fClockData. At the moment you set it up but don't use it, that will be much better than hard coding the values. You can take inspiration from the lifetime correction in the CalorimetryAlg. Units can sometime be a problem so it's worth sanity checking the outputs.
| // TODO: what to do if no lifetime is found? throw an exception?? | ||
| else {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say so. We don't want a mix of corrected & uncorrected values. Theoretically it shouldn't be a problem for any run 1 data right? So it is a good sign to us if it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echo that. In the code throw an exception. In the db use the nearest available value or set a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated thit to use clockdata.
double thit = fClockData->TPCTick2TrigTime(hit.PeakTime()) - t0;
thit = thit * 1.e-3;
Also, updating the line 139 to throw exception if etau is invalid (negative).
else {
std::cout << "sbnd::calo::NormalizeDriftSQLite::Normalize electron lifetime is not found for run " << e.id().runID().run() << std::endl;
throw cet::exception("Electron lifetime is not found");
}
Validation II will serve as a validation...! |
Hi @henrylay97 , thanks a lot for the review! |
henrylay97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sorting the hardcoding Sungbin! I have one lingering comment relating to the changes in develop since this PR was made. Other than that I am happy for this to go in!
| BEGIN_PROLOG | ||
|
|
||
| SBND_Calibration_GlobalTags: { | ||
| @table::TPC_CalibrationTags_Nov2025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now merged, would you have time today to look at how to ensure consistency with this? If not I can look in the morning my time!
Hi @henrylay97 , there will be no consistency issue with CRT's side. Making a separate PR that merges CRT table info into the global tag feature would be good. |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_12_02 SBNSoftware/sbndaq-artdaq-core@v1_10_06 SBNSoftware/sbn*@SBN_SUITE_v10_12_02 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- 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 |
|
approved |
Description
Adding global tag feature to summarize tags of calibration DB tables
sbndcode/Calibration/configurations/for this purpose.sbndcode/Calibration/PDSDatabaseInterface/pmtcalibrationdatabase_sbnd.fclrefers to this directory to collectPMTCalibrationDatabaseTagandDatabaseTimeStamp.Adding time dependent etau correction for data
sbnd_calorimetryalgdata.CaloDoLifeTimeCorrection: falsenot to apply etau correction inlarreco.@local::driftnorm_sqltosbnd_calonormtoolsdataso that etau correction could be made by normtool.sbndcode/Calibration/TPCCalorimetry/NormalizeDriftSQLite_tool.ccperforms etau correction. This module collects etau for each TPC and apply it to the corresponding side of the TPC.Checklist
Reviewers,AssigneesDevelopementRelevant PR links (optional)
Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)?
Link(s) to docdb describing changes (optional)
Is there a docdb describing the issue this solves or the feature added?