Conversation
…& shift for all CRT matchings
This reverts commit 5acaf5e.
henrylay97
requested review from
francescopoppi,
gputnam,
linyan-w and
mrmooney
linyan-w
left a comment
linyan-w
left a comment
There was a problem hiding this comment.
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!
kjplows
moved this from Open pull requests
to Partially reviewed
in SBN software development
|
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 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.
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.
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.
Yes, please do use the xShift
There was a problem hiding this comment.
@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.
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.
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.
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.
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.
Thanks Henry for fixing this!
kjplows
moved this from Urgent checks
to Partially reviewed
in SBN software development
|
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 |
github-project-automation
bot
moved this from To merge
to Done
in SBN software development
linyan-w
moved this from Reviewer Approved
to In tagged release
in SBND 2025 Fall Production
6 participants
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.