-
Notifications
You must be signed in to change notification settings - Fork 35
Calib NTuple CRT Timing Additions #557
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
Conversation
…& shift for all CRT matchings
This reverts commit 5acaf5e.
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.
Thanks a lot Henry!
DocDB entry: https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=43000
Now we have whicht0 = 0 for TPC, 1 for SBND CRT track, 3 for SBND CRT spacepoint, 2 for ICARUS CRT hit. It's not intuitive but reasonably documented in code and the docdb entry, so looks good to me! Thanks!
|
trigger build ci_ref=v10_06_02 LArSoft/lar*@LARSOFT_SUITE_v10_09_00 SBNSoftware/sbnobj#139 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ 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 ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- 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 |
|
❌ 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 |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
@mrmooney @gputnam @francescopoppi just pinging here - from a CI perspective this and #564 (production branch version, on the larsoft v10_06 line) are good to go. Any chance you could look at this? Thanks! |
| double anodeDistance = (hit.PeakTime()-dclock.Time2Tick(dclock.TriggerTime())-time/dclock.TPCClock().TickPeriod())*dclock.TPCClock().TickPeriod()*driftv; | ||
| double wirePlaneX = wireReadout->Plane(geo::PlaneID(hit.WireID().Cryostat, hit.WireID().TPC, hit.WireID().Plane)).GetCenter().X(); | ||
|
|
||
| recoX = wirePlaneX - driftDir*anodeDistance; |
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.
Since xshift is now being passed as a parameter, shouldn't you just apply it instead of calculating recoX here?
Backing up -- tp.x and sp.x should be shifted by the same amount. They shouldn't have different calculations.
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.
Yeah. This second calculation comes from a PR from yourself and Francesco back in March? I was just broadening it to use any CRT T0 rather than just the ICARUS hit tag.
I am happy to pass the xShift, validate the same result is arrived at, and assuming so remove the second calc.
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.
Yes, please do use the xShift
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.
@gputnam @francescopoppi I am seeing ~0mm, ~3mm and ~6mm differences between this technique and pure use of the xshift. Clearly one of the methods doesn't / double accounts for interplane drift time.
I will try and investigate this this afternoon but given you implemented it I would welcome your input - do you know for example, whether this is over corrected?
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, you do want the interplane timing corrected for in the x-shift. The most important thing though is that the tp and the sp are shifted in x by the same amount.
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.
But clearly one of these instances is wrong so it's worth understanding which.
I would expect that the interplane drift is already accounted for in the creation of the spacepoints given they're already 3D constructs. I'm not familiar with the trajectory points so I would need to understand how they work.
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 have looked into this more - SPs explicitly account for the interplane drift as would be expected, TPs are based on the trajectory which itself is a fit to the SPs.
Correcting for interplane drift here is wrong. I've made a couple of plots that show that the SP x's give and agreed x range (with the first induction location being the limit) whilst the TP x's with their current correction give shifted distributions.


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 am therefore going to remove this section and correct all locations by the xshift calculated previously, avoiding double correcting.
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.
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 Henry for fixing this!
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_10_02 SBNSoftware/sbnobj#139 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ 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 |
|
❌ 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 |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- 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 |
Description
Part of a pair of PRs to enhance the SBND CRT contribution to the calibration ntuples. (See also SBNSoftware/sbnobj#139).
This PR:
whicht0==3.Presenting today at the calibration meeting - will link slides after.