Skip to content
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

Shifting CRT T0 and T1 properly in CAFMaker #323

Merged
merged 5 commits into from
Jan 25, 2023

Conversation

jedori0228
Copy link
Contributor

CRT T1 is a time reference to the trigger unlike T0 which is an absolute timestamp. When CRTUseTS0 is set to false and we are saving T1 in CRTHit::time, it should be shift by +srtrigger.trigger_within_gate

@miquelnebot
Copy link
Contributor

@ jedori0228 I assume this goes with
SBNSoftware/sbnobj#72

And these are targeting production but not critical right now for a patch so they will be deferred to next production release early next year. Please say otherwise.

@miquelnebot
Copy link
Contributor

@henrylay97 can you weigh in as in the obj one, please?

@henrylay97
Copy link
Member

Of course. It's not an area of the code I'm familiar with so apologies if my question is naive.

Principle looks fine to me ✔️

I appreciate that this PR purely allows for a different reference time. Its not clear to me where its ensured that MC doesn't have this correction applied? Given this function is applied regardless of whether the event is MC or data? Is the value carefully set elsewhere to avoid this problem?

@jedori0228
Copy link
Contributor Author

@ jedori0228 I assume this goes with SBNSoftware/sbnobj#72

And these are targeting production but not critical right now for a patch so they will be deferred to next production release early next year. Please say otherwise.

Yes, it only affects CAFMaker, and not critical for now.

@jedori0228
Copy link
Contributor Author

Timings in MC is not understood fully yet (at least for me), and we are not doing this shifts for it; ReferenceCRTToBeam() can/should be set to false.

@henrylay97
Copy link
Member

So my worry is that it currently defaults to true, so we would need to actively set it to false in icaruscode & sbndcode fcls

https://github.com/SBNSoftware/sbncode/blob/develop/sbncode/CAFMaker/CAFMakerParams.h#L332-L335

@jedori0228
Copy link
Contributor Author

(Tagging @francescopoppi and @gputnam)
I made few more updates to properly shift and save T0. With FixCRTReferenceTimes(), we initially fill SRCRTHit::t0 (double) with sbn::crt::ts0()/1000. (int64_t). Here we already lose precision. I picked one CRTHit object and printed raw ts0() value and its converted version to SRCRTHit::t0;

crthits[i].ts0()             = 1 656 344 589 827 546 979 (ns)
srcrthits.back().t0          = 1.65634e+15 (us)
                      *1000 -> 1 656 344 585 695 133 696 (ns)

Also, assigning -srtrigger.beam_gate_time_abs/1e3 to double CRT_reference_time caused a problem:

(true value) srtrigger.beam_gate_time_abs = 1656344589830460080 ns
(when saved to a double) -srtrigger.beam_gate_time_abs/1e3 = -1.67904e+16 (us)

Note that the true value is ~1.65e18, so -1.65e18/1e3 should be ~-1.65e15; the order is different, and it comes from the fact that srtrigger.beam_gate_time_abs is unsigned.
To keep all the precision and pass correct reference time, now I shift the timings within Filler function, which was done before v09_63_00_01.

@jedori0228 jedori0228 changed the title Shifting CRT T1 properly in CAFMaker Shifting CRT T0 and T1 properly in CAFMaker Jan 13, 2023
@miquelnebot miquelnebot changed the base branch from release/SBN2022B to develop January 23, 2023 14:53
@miquelnebot
Copy link
Contributor

trigger build SBNSoftware/sbnobj#72

@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@SBNSoftware SBNSoftware deleted a comment from FNALbuild Jan 23, 2023
@miquelnebot
Copy link
Contributor

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_65_01 SBNSoftware/sbnobj#72

@miquelnebot miquelnebot changed the base branch from develop to release/v09_65_03 January 25, 2023 12:09
@miquelnebot
Copy link
Contributor

@FNALbuild
Copy link

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

@FNALbuild
Copy link

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

@FNALbuild
Copy link

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e20: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

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for c7: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

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

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

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link

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

🚨 For more details about the warning 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants